[WIP] Patterns: Add a core selector to get user patterns #55985
Conversation
glendaviesnz
added
[Status] In Progress
glendaviesnz
changed the title
glendaviesnz
force-pushed
the
add/core-user-pattern-selector
branch
from
238eee2 to
3d318fe
Compare
|
@Mamaduka, @youknowriad, @talldan, @kevin940726, @aaronrobertshaw before I spend any time fixing up the typing issues on this PR, does anyone have any questions/concerns about pulling the parsing of the user patterns into a new selector like this. Currently, in the site editor and block editor we are using calls to Memoizing with Let me know if you can see any problems with this approach. |
| @@ -219,6 +222,7 @@ function useBlockEditorSettings( settings, postType, postId ) { | |||
| __experimentalBlockPatterns: blockPatterns, | |||
| __experimentalBlockPatternCategories: blockPatternCategories, | |||
| __experimentalUserPatternCategories: userPatternCategories, | |||
| __experimentalUserPatterns: userPatterns, | |||
There was a problem hiding this comment.
Maybe I'm missing something but the selector was already in block-editor. Meaning it was already a "shared" selector somehow. So I'm wondering why we're adding this new config that probably duplicates some settings that are already there.
There was a problem hiding this comment.
getUserPatterns in the block editor selectors is currently just a private method that is used by the getAllAllowedPatterns selector, so not shareable with site editor currently.
To minimise any backwards compat issues with changing existing selectors in block-editor, and moving away from the use of ReusableBlocks terminology it seemed like a good option was to create a new getUserPatterns selector in core-data and deprecate any selectors that are no longer used in block-editor. This also allows for an invalidateResolution( 'getUserPatterns' ) after adding/deleting user patterns, which is a bit more explicit than invalidateResolution( 'getEntityRecords', ['postType','wp_block] )
However, given that block-editor is already a dependency of edit-site another option is to just modify the selectors in the block editor store and use those in the site editor rather than adding completely new ones in core-data. It might be possible to make use of the getAllAllowedPatterns selector in the site editor instead to achieve something similar. I can take a look at doing that if you think that is a better approach.
There was a problem hiding this comment.
what concerns me the most is that the block editor is already aware of these pattern in its settings and now we're adding another setting that basically is just another format of an existing one.
There was a problem hiding this comment.
Makes sense, will reconsider in light of that.
| @@ -184,58 +184,19 @@ const selectPatterns = createSelector( | |||
| ] | |||
| ); | |||
|
|
|||
| const patternBlockToPattern = ( patternBlock, categories ) => ( { | |||
There was a problem hiding this comment.
I'm guessing this is the duplication that we're removing. Can you explain a bit where we're using this?
There was a problem hiding this comment.
It essentially takes a wp_block entity record and maps it to an object that more closely resembles a theme/directory block pattern in order to allow both to be more easily merged into a single list of patterns under the categories in the site editor patterns page and post editor inserter.
There was a problem hiding this comment.
Don't we have the patterns package now for this kind of things?
There was a problem hiding this comment.
Yeh, maybe instead of pushing new settings into the block editor we can abstract the duplication out into a shared hook in the patterns package, or something. Will play around with some alternative approaches. Thanks for the feedback.
| ); | ||
|
|
||
| return state.userPatterns.map( ( patternBlock: any ) => ( { | ||
| blocks: parse( patternBlock.content.raw, { |
There was a problem hiding this comment.
Calling parse in a selector is potentially a problem because of block lazy loading (see #51778 and #53260). With block lazy loading, parsing content into blocks is an async function because the parser internally lazily loads the blocks it encounters in the content.
And selectors are synchronous, unless you want to also add a getUserPatterns resolver, so they can't really call async functions.
We'll either need to move content parsing out of the selector, or, at least, mark the getUserPatterns selector as experimental/private, so that we're not stuck with it forever.
You can also do a "thought experiment" where the parse function is already async (returns a promise). How does this PR need to change to accomodate that?
There was a problem hiding this comment.
Ah, great to know this, thanks for this feedback, will have another think about the best approach.
|
I am going to explore some alternative approaches based on the feedback so will close this PR for now. |
What?
Still experimental at this stage - exploring options for removing duplication of user patterns code between block editor and site editor by adding a new core selector
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast