Block Quick Inserter: Prioritize showing patterns instead of blocks. #38709
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
jorgefilipecosta
added
[Type] Enhancement
|
Size Change: +762 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
|
This is very cool, thanks for taking a stab! Here's a GIF showing the root level sibling inserter defaulting to patterns: Already here this feels excellent. Of note, I appreciate that you can still search for a block if that's what you want:
One of the concerns discussed in 36697 is whether it feels confusing when the inserter shows two filtered views depending on where you click. But in testing this in practice, it feels reasonably intuitive, enough that we might want to land it so we can benefit patterns more. We should probably not close 36697 with this PR, though, as it might be worth it to still explore the exploded view proposed, which is a larger effort. |
youknowriad
reviewed
Feb 11, 2022
| maxBlockTypes={ SHOWN_BLOCK_TYPES } | ||
| isDraggable={ false } | ||
| __experimentalPrioritizePatterns={ prioritizePatterns } |
There was a problem hiding this comment.
I think this one is an internal component so there's no need for "experimental"
| __experimentalPrioritizePatterns && | ||
| filteredBlockPatterns.length > 2 | ||
| ) { | ||
| maxBlockTypesToShow = 0; |
There was a problem hiding this comment.
Nit: For both filteredPatterns and filteredBlocks, we could bail early if the max is 0 (for perf reasons but really not sure whether it has any impact)
| @@ -98,6 +98,8 @@ export const getSettings = createSelector( | |||
| hasFixedToolbar: isFeatureActive( state, 'fixedToolbar' ), | |||
| __experimentalSetIsInserterOpened: setIsInserterOpen, | |||
| __experimentalReusableBlocks: getReusableBlocks( state ), | |||
| __experimentalPrioritizePatternsOnQuickInserterRoot: | |||
There was a problem hiding this comment.
Nit: I'd be filter for this to be stable preferPatternsOnRoot or something but I'm ok if you think we should start experimental.
There was a problem hiding this comment.
I used the name you suggested "preferPatternsOnRoot" but kept the setting as experimental. We are not yet confident about the future of this change we may in the future have a more general flag that changes more things.
I agree with this, it was always showing footer patterns for me which is not great. |
ntsekouras
reviewed
Feb 11, 2022
| prioritizePatterns: | ||
| settings.__experimentalPrioritizePatternsOnQuickInserterRoot && | ||
| ! rootClientId && | ||
| index > 0 && |
There was a problem hiding this comment.
What about these positions? If I create a new empty template I should see patterns in the inserter, no? Same goes for the last index if it's top level.
There was a problem hiding this comment.
Hi @ntsekouras,
The conditions are following what @mtias specified on the issue:
The top-level means it's a direct descendant of the document, between any root template parts or existing blocks
I added a condition to also show the behavior when the template is empty.
There was a problem hiding this comment.
Yes, we'll need to do some extra work on empty templates regardless.
|
Hi @jasmussen, @youknowriad, @ntsekouras thank you for the reviews. |
…e root of templates
jorgefilipecosta
force-pushed
the
update/prioritize-patterns-on-quick-inserter-root-of-templates
branch
from
dd63911
to
460be3c
Compare
ntsekouras
approved these changes
Feb 17, 2022
There was a problem hiding this comment.
Thanks @jorgefilipecosta!
Personally I find it a bit difficult to follow parts of the flow with some reassigned values like maxBlockTypes becomes maxBlockTypesToShow etc.., but I'm not sure how we can avoid this easily, since we do want the filtered results to make some decisions..
Having said that, this works as described and we can give it a try and see the feedback!
| const showPatterns = | ||
| patterns.length && ( !! filterValue || prioritizePatterns ); | ||
| const showSearch = | ||
| ( showPatterns && patterns.length > SEARCH_THRESHOLD ) || |
There was a problem hiding this comment.
Edge case here in the case of prioritizing patterns where the SEARCH_THRESHOLD is more than SHOWN_BLOCK_PATTERNS_WITH_PRIORITIZATION, in combination of course with only a few allowed blocks. I think it will be a super rare use case if it ever occurs, but just noting.
There was a problem hiding this comment.
Hi @ntsekouras, I think here SHOWN_BLOCK_PATTERNS_WITH_PRIORITIZATION is not relevant, patterns.length contains the full number of patterns e.g: if we have 34 patterns and SHOWN_BLOCK_PATTERNS_WITH_PRIORITIZATION is 2 and SEARCH_THRESHOLD is 3 we are going to show search anyway as expected because we have 34 patterns.
Let me know if I am not understanding correctly the issue.
jorgefilipecosta
deleted the
update/prioritize-patterns-on-quick-inserter-root-of-templates
branch
|
Thank you all for the reviews :) |
cbravobernal
changed the title
cbravobernal
changed the title
cbravobernal
changed the title
cbravobernal
changed the title
|
Hey @jorgefilipecosta @jasmussen and @youknowriad It would be great to take a similar approach to adding inner blocks to for instance the Query Loop block and other blocks where one can add inner blocks. (Would also be very helpful for WooCommerce.) (Paraphrashing/transfering the approach to block where one can add inner blocks.)
Making inner blocks easier to locate. |
This was referenced Apr 19, 2022
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: #36697
This PR follows the design proposed in #36697:
And prioritizes patterns on the quick inserter when all the following conditions match:
By default we have four patterns following the order the server provided them. We don't have any sorting mechanism like last used time, heuristics of the place of insertion, etc, the type of template, etc. As a follow-up, we need to have a mechanism for sorting patterns so what we show is more relevant.
We allow searching for patterns or blocks if we have 3 or more patterns to show we show only patterns if we have 2 or less patterns we show patterns and blocks (patterns being at the top). This allows the user to search for a paragraph block and insert it using the quick inserter if the user wants two (provided there are no 3 or more patterns containing "paragraph" in the name).
Testing
I verified the patterns are prioritized on the inserter on post editor and site editor when the condition are met.