Inserter: Update how we compute the actual insertion point for blocks #65490
Conversation
youknowriad
added
[Feature] Inserter
|
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. |
|
There might a behavior change for blocks with "parent" config (buttons, columns) in this PR, I'll have to look into that. |
|
Size Change: +83 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Ok, this should be ready now. |
youknowriad
force-pushed
the
update/refactor-dynamic-insertion-point
branch
from
44f9a10 to
e65433f
Compare
youknowriad
force-pushed
the
update/refactor-dynamic-insertion-point
branch
from
0f8d6ea to
4831e89
Compare
There was a problem hiding this comment.
The logic here looks a bit clearer to me than what we have on trunk 👍
I was just about to approve it, but then I noticed that when clicking between different blocks in the Blog Home template in TT4, it seems that it's a little slower to switch blocks in this PR than on trunk, while the inserter is open:
Before (trunk)
2024-09-23.16.28.58.mp4
After (this PR)
2024-09-23.16.26.55.mp4
Are you able to see that difference in time to switch blocks? It's a little hard to see in the videos above, but it feels more noticeable to me in my local environment between trunk and this PR. Of note, I had to have the inserter open to experience the slowdown.
| @@ -26,7 +27,7 @@ import { withRootClientIdOptionKey } from '../../../store/utils'; | |||
| */ | |||
| const useBlockTypesState = ( rootClientId, onInsert, isQuick ) => { | |||
| const options = useMemo( | |||
| () => ( { [ withRootClientIdOptionKey ]: ! isQuick } ), | |||
| () => ( { [ noFilter ]: ! isQuick } ), | |||
There was a problem hiding this comment.
While reading over this PR, it took me a little while to understand what noFilter meant, possibly because of the double negatives? Totally optional, but what about a positive isFiltered?
| * Internal dependencies | ||
| */ | ||
| import items from './fixtures'; | ||
| import BlockTypesTab from '../block-types-tab'; |
There was a problem hiding this comment.
Without having had a look at the Native code for this component, why is this test being removed? I wasn't sure how it relates to the rest of the PR, or is it just a code cleanup?
There was a problem hiding this comment.
The test was failing and then I looked into fixing it and it involves mocking too much IMO and the test itself doesn't do much. Do you see any value in this test?
There was a problem hiding this comment.
Gotcha, thanks for the extra context!
Do you see any value in this test?
Nope, it looks pretty simple (just tests that it renders) and adding blocks is a prominent part of the mobile app, and is testing well manually with this PR:
This sounds okay to remove to me 👍
|
To be honest for the performance thing, I'm not sure if I'm seeing any difference personally. It doesn't translate into the "focus" metric that's for sure. I would love to see other tests to see if we can confirm/infirm it. |
youknowriad
force-pushed
the
update/refactor-dynamic-insertion-point
branch
from
af106ca to
d728620
Compare
|
Flaky tests detected in d728620. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10991507537
|
There was a problem hiding this comment.
We have a bug on this PR.
If there is CPT locking and insert is not possible at all:
function custom_registered_post_type_post( $post_type, $post_type_object ) {
$post_type_object->template = array(
array( 'core/heading' )
);
$post_type_object->template_lock = 'insert';
}
add_action( 'registered_post_type_post', 'custom_registered_post_type_post', 10, 2 );
We are now showing the inserter with all the blocks as if inserting was possible On the trunk we show the inserter but without any blocks to insert. This is also not ideal we should not show the inserter at all, as happened previously but the behavior of the trunk is still better.
|
@jorgefilipecosta Good catch. Personally I'm not sure if trunk is better. I'm actually wondering if showing all the blocks is actually better. This is related to what I mention in the PR description. If you check this PR, the blocks tab now behaves like the reusable blocks tab, you try to insert but nothing happens. I think maybe a better solution here is to show a notice: "You can't insert this block" or something like that. Any thoughts @jasmussen @jameskoster |
|
I'd tend to agree that showing in a grayed or otherwise disabled state the blocks you can't insert may indeed communicate better, that sure, they can't be inserted. The challenge here is that of always having a populated inserter, so that you never end up with a "No blocks found" state, regardless of what block you intentinally or accidentally selected in the site editor first. I.e. this is technically intended:
If you can insert inside the list item, do that. If you can't, append after.
Am I understanding correctly, that in this PR, the steps above are still intact, but on top of that you're suggesting adding this "can't be inserted" and/or graying the blocks, in the case of a CPT that locks down the post/template from having any blocks inserted at all? |
yes. I prefer not to "gray out" the blocks because that we'll have to compute whether we can insert a block before hand (when rendering the list) which can be bad performance wise. While if we do that upon click it's easier as we'll only compute it for a single entry. |
|
I can see a "can't be inserted because a/b/c" can be a good message to show. You can be verbose in the helper text too. |
Re-testing with a fresh install in WP Playground I'm not seeing much of a difference, either. Maybe it was just my testing environment when I was looking at this yesterday. Not a blocker IMO if we're finding the perf difference hard to reproduce, and we can always look into it after the fact if need be 👍 |
There was a problem hiding this comment.
Showing a message saying the block can't be inserted addresses my concern. So I think this PR is good to merge.
I wonder if as an enhancement, when hovering over an item that is not possible to be inserted at all, we could grey out the item and have a tooltip saying it is not possible to insert an item. But that is only a possibility to consider and not a blocker.
youknowriad
force-pushed
the
update/refactor-dynamic-insertion-point
branch
from
91cac7f to
a386831
Compare
|
I have reported an issue resulting from this PR. |
Related #60021
What?
In a previous PR #62169 we updated the block tab in the inserter to always show the full list of blocks even if we're not allowed to insert in the current position (for instance within contentOnly blocks...) and for each block, we computed a destination clientId dynamically.
The implementation was not ideal IMO for two reasons:
For these reasons, this PR refactors that implementation and introduces a
getClosestAllowedInsertionPointselector that can be run late during the onHover or onSelect callbacks. As a follow-up to the current PR I'm also planning to use that selector (or a modified version of it) to implement the same behavior for patterns (always show all patterns and only compute the insertion point on hover/insert for a given pattern).Testing Instructions
1- In the site editor or the post editor add a list block
2- Add a list item
3- Open the inserter
4- Notice all blocks are listed
5- Insert a block that is not the list item block, e.g. an image block
6- Notice the image block is inserted right after the parent list block of the currently selected list item block