Inserter: show all blocks (alternative) #62169
Merged
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
|
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
|
draganescu
reviewed
May 31, 2024
There was a problem hiding this comment.
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
itemsRemainingshows 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 |
ellatrix
commented
May 31, 2024
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! |
ellatrix
commented
May 31, 2024
| item.rootClientId, | ||
| getIndex( { | ||
| destinationRootClientId, | ||
| destinationIndex, |
There was a problem hiding this comment.
At some point we should remove all this destination stuff from and calculate it here inline, it would simplify things.
ellatrix
commented
May 31, 2024
| item.rootClientId = rootClientId; | ||
|
|
||
| while ( | ||
| ! canInsertBlockTypeUnmemoized( |
There was a problem hiding this comment.
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.
ellatrix
commented
May 31, 2024
| @@ -2039,13 +2039,52 @@ export const getInserterItems = createRegistrySelector( ( select ) => | |||
|
|
|||
There was a problem hiding this comment.
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.
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.
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.
Made it a private option in 9a3f379
draganescu
reviewed
May 31, 2024
|
Great work, this is a substantially better experience. I’d recommend:
With that in place I think we can ship this. |
draganescu
changed the title
draganescu
approved these changes
May 31, 2024
There was a problem hiding this comment.
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.
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.
I think adding a comment of how the discovery algorithm works would be great to read in a couple of months :D
|
There's some duplication if you have the 'most used blocks' preference toggled on: 
|
draganescu
added
[Type] Bug
|
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:
|
|
If Most Used is rendered for the allowed blocks, could we not render the second one? Here's the latest with the Nav block:
|
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). |
richtabor
approved these changes
May 31, 2024
|
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 |
|
The Screenshot
|
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jun 4, 2024
* 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 <ellatrix@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: SaxonF <saxonafletcher@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org>
|
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 ) |
|
Hey there 👋 We are investigating an issue on the native editor with the Social icons block, when tapping on the inline inserter, all of the blocks are shown with the social icons:
When testing the Web editor with these changes, when tapping on "Browse all blocks" all blocks are available as well (although after the social icons). Is this expected? Web editor (Before)Before.movWeb editor (After)After.movConfirming if it is expected would help us move forward with a fix on the native side. Thank you! |
|
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! |
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
* 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 <ellatrix@git.wordpress.org> Co-authored-by: draganescu <andraganescu@git.wordpress.org> Co-authored-by: jeryj <jeryj@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: richtabor <richtabor@git.wordpress.org> Co-authored-by: SaxonF <saxonafletcher@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org>
This was referenced Aug 10, 2024
This was referenced Sep 3, 2024
This was referenced Sep 19, 2024
6 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.
What?
Fixes #60991
This is an alternative to #61873.
This PR changes
getInserterItemsto 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