Show patterns in inserter for non-root level insert destinations #28459
Conversation
This file contains 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
|
Size Change: +228 B (0%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
david-szabo97
added
the
[Feature] Inserter
david-szabo97
requested review from
vindl,
mattwiebe,
Addison-Stavlo,
Copons,
jeyip and
youknowriad
Comment on lines
1754
to
1770
| const patternsAllowed = filter( patterns, ( { content } ) => { | ||
| const blocks = parse( content ); | ||
| return every( blocks, ( { name } ) => | ||
| canInsertBlockType( state, name, rootClientId ) | ||
| ); | ||
| } ); |
There was a problem hiding this comment.
I wonder if checking every block of every pattern could be a bit cumbersome if we are running a large amount of available patterns? 🤔 While it seems the most accurate way to handle this decision, I wonder if just whitelisting the blocks that should allow patterns as children would make more sense as it would be a much more simple check. Is there a need to check every block, or do all parent blocks that should allow patterns support all the same children used in patterns?
There was a problem hiding this comment.
Yep, I was thinking about how we could make this better perf-wise. createSelector takes care of memorizing, but parsing the content of every pattern every time we select a different block is definitely cumbersome. Note, the check here only checks for the first level of blocks which means inner blocks aren't checked. Which makes sense to me.
So far my idea is to precompute the allowed patterns for each block. Then we can save it somewhere and just load it when the block editor is opened.
I'm not sure yet 🤔
There was a problem hiding this comment.
Also, reworking the selector to memoize by block type rather than client id should be a great save as well.
youknowriad
reviewed
Jan 26, 2021
| } | ||
|
|
||
| const patternsAllowed = filter( patterns, ( { content } ) => { | ||
| const blocks = parse( content ); |
There was a problem hiding this comment.
Parsing is a heavy operation and doing on a selector (which is actually the bottleneck for redux applications) is something we should consider very carefully. Did you notice any impact on the performance of the patterns tab? Do we have any measures about that tab?
There was a problem hiding this comment.
So far it's adding up +100ms to the loading of the inserter each time I select an element with a different parent. Local dev installation with TT1B theme. In production, it will be a bit faster but way too much either way, and don't forget my dev machine is probably beefier than the average users'. This is definitely unacceptable if we have more patterns.
Was there any exploration about memoizing/implementing a cache for parse?
There was a problem hiding this comment.
Just pushed a commit which creates a selector for parsed patterns. Now the parsing is only done once. So only the first open of the inserter is affected.
david-szabo97
force-pushed
the
add/allow-patterns-for-non-root-inserts
branch
from
cf4521e
to
cd0c727
Compare
youknowriad
reviewed
Jan 28, 2021
| @@ -125,6 +125,9 @@ function useBlockEditorSettings( settings, hasTemplate ) { | |||
| ); | |||
| const { canUser } = select( coreStore ); | |||
|
|
|||
| // Prefetch and parse patterns. | |||
| select( 'core/block-editor' ).__experimentalGetAllowedPatterns(); | |||
There was a problem hiding this comment.
This is very weird to me to be honest. Can we at least comment that it's a performance trick?
There was a problem hiding this comment.
I was trying to figure out a better location for this snippet. I'd be happy to move this somewhere else but I have no idea where to 🤔 I'll update the comment.
|
Moved the prefetch to |
|
This is working well for me now and I think it could make sense for the first iteration. It needs a rebase against master to pull in some latest e2e test fixes that are failing in this PR. Since the block parsing that's done once on editor load is a potential performance concern with this approach, I'll just throw in a couple of rough ideas that could be done in a follow up:
|
david-szabo97
force-pushed
the
add/allow-patterns-for-non-root-inserts
branch
from
64b334a
to
7b191de
Compare
vindl
approved these changes
Feb 3, 2021
Merged
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #25915
Description
Patterns are only allowed to be inserted on the root level. (as a child of Document)
This PR adds the option to insert patterns wherever it's possible.
It is based on two new selectors:
__experimentalGetParsedPatterns: Parses all the patterns and memorizes the value. Thus we are only parsing patterns once per page session.__experimentalGetAllowedPatterns: To avoid corrupting the content, we are only allowing to insert patterns that contain blocks that are allowed to be inserted into our target block. This function filters the parsed patterns and returns only the patterns that can be inserted.Since
parseis a heavy operation, we are memorizing the parsed patterns. To avoid affecting the time it takes to open the inserter, we are calling the__experimentalGetAllowedPatternsfunction after loading the editor. This will cause the editor to take longer to load but throughout the usage of the block editor, we won't notice any performance downgrades.This solution isn't ideal, but it gets the balls rolling.
How has this been tested?
Columns, choose 50 / 50Paragraphinto the first column with some random textParagraphblockColumnblock of theParagraphScreenshots
Screen.Recording.2021-01-25.at.16.04.30.mov
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: