-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inserter: show all blocks (alternative) #62169
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: -29 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of this PR compared to #61873 it's centralising the decision around where a block should be inserted right at the core of the process, when getting the list of blocks to be shown in the inserter.
There are some missing parts compared to the alternative though:
- the logic needs to be updated so that the insertion is always visually after the currently selected block, just like normal insertion is
- for some reason the insertion point for blocks in
itemsRemaining
shows up in the site editor but not in the post editor - this PR does not address the situation when parents cannot be used for insertion so children have to be tested
- when a page is edited in templated locked mode we cannot find an insertion point up the tree we need to go down the tree, in a way in which we don't specify a block by name (e.g. post-content).
- the original PR solves it 50% in the sense that the insertion happens in the 1st available spot, which is really not intuitive at all. Maybe we should not fix this yet?
Could you elaborate here? I think I am always inserting it after the selected block, I'm not sure what you mean :)
Can we fall back to section root client ID instead of the absolute root |
packages/block-editor/src/components/inserter/block-types-tab.js
Outdated
Show resolved
Hide resolved
Hmm, in my original testing the block got inserted before the selection 🤷🏻 Now trying to record a video after latest updates it works fine ... |
That could work too yes! |
item.rootClientId, | ||
getIndex( { | ||
destinationRootClientId, | ||
destinationIndex, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point we should remove all this destination
stuff from and calculate it here inline, it would simplify things.
item.rootClientId = rootClientId; | ||
|
||
while ( | ||
! canInsertBlockTypeUnmemoized( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting that, if you can select any block at the selected block, it doesn't impact the performance of this selector (and thus inserter opening) at all, because we already had this check and this loop will stop immediately. For other cases, it will loop up the tree until we find a place where it can be inserted. At some point we should investigate getBlockEditingMode
because that's probably a very unperformant selector.
@@ -2039,13 +2039,52 @@ export const getInserterItems = createRegistrySelector( ( select ) => | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getInserterItems
is a public selector, so we might have to create a private selector and make changes to that one instead 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Is this change bound to break anything? It should improve the UX across the board. The normal classic functionality remains as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use getInserterItems
it will now show all blocks, but with a rootClientId
property, so the handling is different, technically a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it a private option in 9a3f379
Great work, this is a substantially better experience. I’d recommend:
With that in place I think we can ship this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code simplifies the approach in the alternative PR (#61873) and from my reading it only adds the least needed amount of work when needed. By using a private option there is no public API change so we can safely land this.
This still introduces patterns in search results for blocks that hid them previously, for instance in a pattern with overrides, but they were allowed in search results in trunk
except they were filtered out b/c of no insertion point. Now they can be inserted so they show, it is not a change of behavior.
onInsert | ||
); | ||
|
||
export function BlockTypesTabPanel( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is added because we now split the blocks tab in two panels, one for blocks inserted in the current selection insertion point one that can be inserted in the nearest available insertion point relative to the current selection.
) | ||
.map( buildBlockTypeInserterItem ); | ||
|
||
if ( options[ withRootClientIdOptionKey ] ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding a comment of how the discovery algorithm works would be great to read in a couple of months :D
This is an edge case, so we might address separately but; if you're editing a page with a template that contains no Content block, then the Inserter will still turn up empty. Ideally there should be some messaging explaining what's going on. Something like:
|
Right, but even with a long allowedList, there probably shouldn't ever be multiple "Most Used" suggestions.
Sounds good, I think tweaking most used in those scenarios with lots of blocks could be a potential follow-up fix. The HR styling as well (double-border). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A solid experience win. Thanks for everyone who pushed this forward.
This PR caused a pretty big regression in the
It's both on this PR branch and on trunk after merging, so the regression is real, not some random breakdown. The metric is calculating how long does it take to dispatch the |
* Inserter: show all block * Fix parents * Use section root as fallback * Remove heading, remove reusable blocks * Add private selector option * Fix quick inserter * Fix appender inserter * Fix most used, fix quick inserter * Fix widgets page * Fix e2e tests Co-authored-by: ellatrix <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: SaxonF <[email protected]> Co-authored-by: annezazu <[email protected]>
Looks like this impacted the "inserter hover" metric. |
@youknowriad You're late to the party, it's already been fixed 😛 #62263 (comment) |
haha :) I wish all perf regressions were like that :P (fixed before I even notice them :P ) |
After going through some of the linked issues, it looks like it is indeed the desired behavior 👍 We'll work on how we sort the blocks in the inserter. Sorry for the noise! |
* Inserter: show all block * Fix parents * Use section root as fallback * Remove heading, remove reusable blocks * Add private selector option * Fix quick inserter * Fix appender inserter * Fix most used, fix quick inserter * Fix widgets page * Fix e2e tests Co-authored-by: ellatrix <[email protected]> Co-authored-by: draganescu <[email protected]> Co-authored-by: jeryj <[email protected]> Co-authored-by: jasmussen <[email protected]> Co-authored-by: jameskoster <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: richtabor <[email protected]> Co-authored-by: SaxonF <[email protected]> Co-authored-by: annezazu <[email protected]>
What?
Fixes #60991
This is an alternative to #61873.
This PR changes
getInserterItems
to always return all items, but each item defines at which root client ID it can be inserted.Inside the inserter, when a block is selected, there's a list for blocks that can be inserted at the selection, and a list with remaining blocks. Since each of these blocks have a root client ID, we know where to show the insertion point. The index at which to insert is derived from the selection.
Why?
See #60991.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast