Fix empty Patterns inserter when selecting section root when editing Pages in Site Editor #66214
Conversation
|
Size Change: +32 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
getdave
changed the title
|
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. |
|
@talldan I'm not 100% on this one. It changes the model for Patterns insertion on a specific case for the section root block when selected. The model for this one block is that if selected it will look up Patterns for that block and insert them within that block (as opposed to look up the selected block's root block). |
Out of interest, why do users need to be able to select the post content block when in zoom out mode? |
I believe last time it was discussed it was to allow for background or width to be changed or something like that. But that seems like a bit of an edge case. I wonder if it was just necessary to allow the Patterns inserter to be populated. If we disabled the section root block then presumably the Patterns inserter would be empty a lot of the time when in the Site Editor. I would like to point out however, that even when not in Zoom Out mode this still fix may still be required as users can still arrive at the "empty" inserter. |
| const sectionRootClientId = getSectionRootClientId(); | ||
|
|
||
| if ( sectionRootClientId === selectedBlockClientId ) { | ||
| _destinationRootClientId = sectionRootClientId; | ||
| _destinationIndex = getBlockOrder( | ||
| _destinationRootClientId | ||
| ).length; |
There was a problem hiding this comment.
So as you described, the inserter is empty is because it usually inserts after the selected (post content) block, but the block list that the post content block is in is locked.
The code here detects that case for the section root client id (which is again the post content block in this case) and special cases it to insert within that block instead of after it.
Are there any other cases where this might be a problem? In some cases, sectionRootClientId can be the block that's the <main> element, so I think this logic might also apply there. Also noting that I think this logic is applied regardless of whether zoom out is active or not.
I wonder if it's something that can be made more generic - that if there's a container block selected and we can't insert after it (the root is templateLocked or blockEditingMode is non-default), then instead we try inserting inside it. This would be quite a fundamental change in the middle of the 6.7 cycle, but it might also be safer and have less edge cases.
The alternative could be that the special case is kept, but it's only active during zoom out mode, which I don't think would cause any issues.
I think separately the empty state of the pattern inserter could be improved, because there are probably cases where it's legitimately impossible to insert blocks and it might be empty (e.g. if entire posts or pages are locked).
There was a problem hiding this comment.
The alternative could be that the special case is kept, but it's only active during zoom out mode, which I don't think would cause any issues.
I think this might be the best approach.
I agree that changing the way Pattern insertion works at this stage in the release is too large a change to feel comfortable.
There was a problem hiding this comment.
Are there any other cases where this might be a problem? In some cases, sectionRootClientId can be the block that's the
element, so I think this logic might also apply there.
Yeah, I can repro this, it does insert inside the <main>, but most of the time I think it's where the user would want to insert, so maybe it's better. It just makes the insertion behavior a little inconsistent!
There was a problem hiding this comment.
I updated to target Zoom Out only.
|
@getdave I made a PR that hides the slide out panel whenever there are no categories. It should work alongside your fix (e.g. if you decide only to apply this logic when zoom out is active as discussed, it'll improve the UI when zoom out isn't active). |
|
Nice! I just updated here and will review yours Monday 👍 |
jeryj
force-pushed
the
fix/use-correct-patterns-destination-root-in-zoom-out
branch
from
b422ce0 to
185b059
Compare
|
Rebased since so much had changed on wp/6.7 branch with zoom out |
There was a problem hiding this comment.
I don't think this fixes the issue, just makes it less common to encounter the bug. I believe backporting #65611 is the right fix. We should always show all patterns and insert them at the next best place. In the instance of this PR, the next best place is the end of the block list.
For example, you can still get to a place fairly easily where no patterns are shown. While it is the expected state given the code, an empty pattern directory is surprising as a user since it's not clear why it should be empty.
| @@ -92,10 +94,26 @@ function useInsertionPoint( { | |||
| // Insert after a specific client ID. | |||
| _destinationIndex = getBlockIndex( clientId ); | |||
| } else if ( ! isAppender && selectedBlockClientId ) { | |||
There was a problem hiding this comment.
Just adding __unstableGetEditorMode() !== 'zoom-out' to this conditional is enough to get the right things you want. You don't need to set the _destinationRootClientId = sectionRootClientId; later on, as it should be the same or not matter.
There was a problem hiding this comment.
So this PR would just become
| } else if ( ! isAppender && selectedBlockClientId ) { | |
| } else if ( ! isAppender && selectedBlockClientId && __unstableGetEditorMode() !== 'zoom-out') { |
There was a problem hiding this comment.
Unless I'm confused, I don't think we can do that.
This code block needs to run even if not in zoom out. It's only setting destination* variables that's different in zoom out.
getdave
added
Backport to WP 6.7 Beta/RC
|
Thanks for your work here @jeryj and for the review. Much appreciated.
Agreed and I share your concerns. However, this PR is a targeted fix for 6.7 only to try and avoid a very common UX issue with selecting the
I'm nervous about this because that PR relies on another PR which seems to make some pretty significant changes. Is it possible to backport the PR you suggest without introducing other fundaemental changes. It's difficult - I'm trying to find a balance here. We are trying to fix a UX issue which is important but not critical. I don't want the optimal fix to end up introducing other potential problems into the release. |
There was a problem hiding this comment.
IMHO, this PR in general makes the UX a little bit better. Even though there are still some bugs, I think it's better than nothing. Since 6.7 is rapidly approaching, I think we can land this together with #66246.
getdave
deleted the
fix/use-correct-patterns-destination-root-in-zoom-out
branch
What?
Fix empty Patterns inserter when
core/post-contentis selected when editing aPagein the Site EditorFixes #65833
Why?
As detailed in the Issue, it's possible to select the
core/post-contentblock when editing Pages in the Site Editor. This is expected functionality for Zoom Out.However, when selecting this block the Patterns inserter shows "empty" because the block root for the block is part of the Template and is thus disabled.
This provides a confusing UX because this section is acting as the defacto "root" of the canvas and thus when selected it should:
The problem is:
rootClientIdis provided by the variabledestinationRootClientIdreturned from theuseInsertionPointhook which gets passed to<PatternCategoryPreviews>useInsertionPointmeans that the_destinationRootClientIdis set by callinggetBlockRootClientId(selectedBlockClientId). This returns the root block from which the block is nested which is then used to determine which Patterns are elligable. Remember it is this value which ultimately gets passed to__experimentalGetAllowedPatterns()asrootClientIdgetBlockRootClientId(selectedBlockClientId)is sometimes part of the Template and is thusdisabledviagetBlockEditingMode. Typically it's the<main>block that wrapsHeader,Post ContentandFooter.How?
What we need to do is ensure that if the selected block is the
sectionRootClientIdthen we consider it to be the "root block" for which to look up eligible Patterns.When a Pattern is inserted, we set the insertion index to be at the end of the section root block.
Testing Instructions
core/post-contentblock.Testing Instructions for Keyboard
Screenshots or screencast
Before
Screen.Capture.on.2024-10-18.at.10-32-47.mp4
After
Screen.Capture.on.2024-10-18.at.10-31-17.mp4