Block editor settings: delay selecting pageOnFront, pageForPosts #57290
Conversation
packages/block-editor/src/components/link-control/use-search-handler.js
Outdated
Show resolved
Hide resolved
|
Size Change: +1.35 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 17857cd. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7287327829
|
| @@ -229,8 +235,7 @@ function useBlockEditorSettings( settings, postType, postId ) { | |||
| // Check these two properties: they were not present in the site editor. | |||
| __experimentalCreatePageEntity: createPageEntity, | |||
| __experimentalUserCanCreatePages: userCanCreatePages, | |||
| pageOnFront, | |||
| pageForPosts, | |||
There was a problem hiding this comment.
I think these settings were meant to be experimental. I think we should be ok with removing it, thinks will just keep working fine with undefined.
ellatrix
requested review from
youknowriad,
MaggieCabrera,
ntsekouras,
getdave and
jorgefilipecosta
ellatrix
requested review from
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel and
kevin940726
as code owners
| // The function should either be undefined or a stable function reference | ||
| // throughout the editor lifetime, much like importing a function from a | ||
| // module. Maybe warn if this becomes a common pattern and it does change? | ||
| const { pageOnFront, pageForPosts } = usePageSettings?.() ?? {}; |
There was a problem hiding this comment.
Is this the only place where we need these settings? It seems we only use them in a callback, so maybe a better settings would be a callback getSiteSettings or something like that no?
There was a problem hiding this comment.
Also something else, I've noted, it seems that the "link control" component relies on a lot of settings (__experimentalFetchLinkSuggestions) for instance and all these settings are not used elsewhere (unless I'm wrong). So maybe it's the way we're implementing the link control that is wrong, maybe we should just have a "default link control" and make it a filterable component like "mediaUpload". (Just thinking of alternatives, I'm not sure which is best)
There was a problem hiding this comment.
Yes, I think we should consider that too. I'm however not familiar enough with it to want to attempt a complete rewrite 😄
There was a problem hiding this comment.
I'm trying to make things better step by step and make sure are the dependencies are completely private so it's easier to rewrite in the future
There was a problem hiding this comment.
It seems we only use them in a callback, so maybe a better settings would be a callback getSiteSettings or something like that no?
No, because it's used for rendered UI, not a callback. The selector is a resolver, so the component should re-render when the settings come in.
There was a problem hiding this comment.
It seems like useSearchHandler could entirely be a setting, but I'd prefer to attempt this separately because it would mean duplicating all this code to edit-widgets.
There was a problem hiding this comment.
Also something else, I've noted, it seems that the "link control" component relies on a lot of settings (__experimentalFetchLinkSuggestions) for instance and all these settings are not used elsewhere (unless I'm wrong). So maybe it's the way we're implementing the link control that is wrong,
@youknowriad I'd be grateful to hear more about how you'd like LinkControl to change. Now is the time because a lot of demands are being placed on it.
...maybe we should just have a "default link control" and make it a filterable component like "mediaUpload". (Just thinking of alternatives, I'm not sure which is best)
I'd like to understand more about this.
| export { default as __experimentalFetchUrlData } from './__experimental-fetch-url-data'; | ||
|
|
||
| export function __experimentalUseLinkControlEntitySearch() { |
There was a problem hiding this comment.
Can this be a private API instead of a prefixed one.
There was a problem hiding this comment.
I am working on a plan to move this fetching to become a full selector in Core Data in order to take advantage of caching...etc. So making it private would be good.
There was a problem hiding this comment.
In theory I don't see a problem with this approach if it helps optimise the editor by reducing number of upfront settings calls.
Obviously it introduces yet more complexity around fetching of results.
We'll also need to fix the mocks in the tests as it seems they are all broken.
Ready and willing to collaborate on improvements to LinkControl so please do keep me in the loop.
| export { default as __experimentalFetchUrlData } from './__experimental-fetch-url-data'; | ||
|
|
||
| export function __experimentalUseLinkControlEntitySearch() { |
There was a problem hiding this comment.
I am working on a plan to move this fetching to become a full selector in Core Data in order to take advantage of caching...etc. So making it private would be good.
| return result; | ||
| } else if ( Number( result.id ) === pageForPosts ) { | ||
| result.isBlogHome = true; | ||
| return result; |
There was a problem hiding this comment.
Don't think we need these early return right?
| return result; |
| ).map( ( result ) => { | ||
| if ( Number( result.id ) === pageOnFront ) { | ||
| result.isFrontPage = true; | ||
| return result; |
There was a problem hiding this comment.
| return result; |
I think we can remove these early returns
| @@ -229,8 +235,7 @@ function useBlockEditorSettings( settings, postType, postId ) { | |||
| // Check these two properties: they were not present in the site editor. | |||
| __experimentalCreatePageEntity: createPageEntity, | |||
| __experimentalUserCanCreatePages: userCanCreatePages, | |||
| pageOnFront, | |||
| pageForPosts, | |||
| // The function should either be undefined or a stable function reference | ||
| // throughout the editor lifetime, much like importing a function from a | ||
| // module. Maybe warn if this becomes a common pattern and it does change? | ||
| const { pageOnFront, pageForPosts } = usePageSettings?.() ?? {}; |
There was a problem hiding this comment.
Also something else, I've noted, it seems that the "link control" component relies on a lot of settings (__experimentalFetchLinkSuggestions) for instance and all these settings are not used elsewhere (unless I'm wrong). So maybe it's the way we're implementing the link control that is wrong,
@youknowriad I'd be grateful to hear more about how you'd like LinkControl to change. Now is the time because a lot of demands are being placed on it.
...maybe we should just have a "default link control" and make it a filterable component like "mediaUpload". (Just thinking of alternatives, I'm not sure which is best)
I'd like to understand more about this.
getdave
added
the
[Feature] Link Editing
What do you mean? Imo this is clearer and simpler, at least I don't see any added complexity. |
| return useCallback( | ||
| async ( val, suggestionsQuery ) => { | ||
| return ( | ||
| await fetchLinkSuggestions( val, suggestionsQuery, settings ) |
There was a problem hiding this comment.
Could we instead access the original settings.__experimentalFetchLinkSuggestions and call that if it's defined? That might provide backwards compatibility we need.
Extenders are likely changing link control by overwriting the I felt the complexity was higher because now:
That's more indirection and complexity IMO. If I'm trying to follow this flow then there's several steps of indirection I must follow to work out where the data actually comes from (i.e. Nonetheless, this (debatable) complexity may be offset by the other advantages - as I indicated in my original review. That aside, I think the main problem could be backwards compatibility. The original extension point of I appreciate this is |
|
Let's wait with this for a bit. I think we can find a good way to make everything functions instead of passing values and use resolvers in block-editor. |
|
@ellatrix I started looking at moving the link control results to Core Data resolvers. LMK if that's in line with what you were thinking. |
What?
Instead of fetching site setting on page load and updating the block editor store, let's delay fetching the information until it's needed.
So what I'm doing in this PR is: instead of selecting and passing the result as a block editor setting, I'm passing a function (hook) to allow components to subscribe to the information themselves. For this to work correctly, the hook must remain a stable function of course, just as any other hook. If this becomes a more used pattern, we could consider warning if it does change.
This is part of a larger effort to reduce fetching information for block editor settings. The idea is to do something similar for patterns and reusable blocks: only fetch them when it's needed, instead of on page load.
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast