Fix unwanted rich previews for internal urls#32613
Closed
getdave wants to merge 4 commits into
Closed
Conversation
getdave
added
[Feature] Link Editing
|
Size Change: +38 B (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jun 11, 2021
| const { getSettings } = select( blockEditorStore ); | ||
| return { | ||
| baseUrl: getSettings().__experimentalBaseUrl, | ||
| fetchRemoteUrlData: getSettings().__experimentalFetchRemoteUrlData, |
Contributor
There was a problem hiding this comment.
So If I'm not wrong, this feature requires two block-editor settings, It feels a bit too much for just previews tbh. Can't we introduce the bail out behavior inside __experimentalFetchRemoteUrlData somehow?
Contributor
Author
There was a problem hiding this comment.
That was my first thought. Should that need to know about the concept of an internal URL though?
I can look into that later on today.
Contributor
There was a problem hiding this comment.
Maybe the block-editor doesn't need to know about "fetchRemoteURL" entirely? I'm just thinking out loud but maybe the setting should be renderURLPreview (render function)
Contributor
Author
There was a problem hiding this comment.
That is an interesting option.
Member
There was a problem hiding this comment.
But does it mean that the users have to implement the loading and error status/style by themselves?
How about instead of accepting an async function (fetchRemoteUrlData), or a render function (renderUrlPreview), we simply accept a React hook (useRemoteUrlData) as the setting value? The hook takes a remote url as the argument, and return richData and isFetching, just like the one we've already created.
useRemoteUrlData( url ) {
// ...
return { richData, isFetching };
}This way, the users don't have to implement the loading style, and they can also skip fetching internal URLs if the url matches a certain pattern. There are no intermediate loading status like we have in async function since that everything can return synchronously. In the future, we can also handle errors or support other progressive enhancements directly in the hook.
Contributor
There was a problem hiding this comment.
so it would be something like:
const { useRemoteURLData } = useSelect( select => select( 'block-editor' ).getSettings(), [] );
const { richData, isLoading } = useRemoteURLData();
this kind of breaks the rules of hooks, because that hook is not necessarily defined and can be updated over time to call different built-in hooks.
I do think it could be nice, but maybe a bit fragile and a bit weird as well (a react hook as a setting).
WDYT
Contributor
Author
There was a problem hiding this comment.
@youknowriad What you have is (I think) how @kevin940726 and I imagined it.
because that hook is not necessarily defined
That's probably the biggest problem for me. It's likely to only be defined in the Editor and not the Block Editor.
Contributor
Author
There was a problem hiding this comment.
How about we change the setting to fetchRichData and remove the current __experimentalFetchRemoteUrlData.
This will accept a url much as __experimentalFetchRemoteUrlData does now. But within the function, we can decide which path to follow based on whether the url is internal or not.
Initially the implementation would be something like:
function __experimentalFetchRichData(url) {
if (url.includes(baseUrl)) {
return Promise.resolve({})'
}
// Now do the same as `__experimentalFetchRemoteUrlData` does currently
// to handle external URL
}Then in the future we could bolt on the ability to fetch rich data about an internal URL very easily.
This allows us to keep the current hook implementation the same whilst also making sure that block-editor has no idea about WP specific stuff.
Member
There was a problem hiding this comment.
What I was thinking is to split <LinkPreview> into 2 different components, one is the original LinkPreview, and the other one is the LinkRichPreview component. We can skip rendering <LinkRichPreview> if there's no useRemoteURLData (or any other name) so that we don't break the rules of hooks. And I would imagine the setting can't be updated once it's rendered so that it won't break the other rule of hooks either.
But I agree that it's a bit weird though. Maybe render function (or just a component) is a better way to go, but would need to update LinkPreview to make it work.
Member
|
This may have been answered somewhere else, but why don't we want to show previews for internal links? It seems like there is still value. |
Contributor
Author
Great question. We do but we should do it with another mechanic which grabs the necessary data. After all we should have all the data for rich prevuews by querying WP. This was going to be one for a follow up. |
Contributor
Author
|
Closing in favour of #32658 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Due to the introduction of loading placeholders towards the end of #31464, internal URLs will now briefly showing a "loading" state. This is not desireable.
This PR fixes this by testing against a base URL which is used to test if the URL being previewed is an internal URL. If it is then we bail out.
This
baseURLhas been added to the@wordpress/editorblock editor settings as we cannot apply the test directly in<LInkControl>as this must remain WordPress agnostic. Note however, that the concept of a internal URL applies to all types of application.Closes #32657
Questions
baseURLto the settings on the server side to ensure it's populated "early" and doesn't require a fetch?How has this been tested?
www.wordpress.org).Screenshots
Before
After
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).