Navigation: Try removing absorb toolbar prop. #36990
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
|
Size Change: +397 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
getdave
reviewed
Nov 30, 2021
There was a problem hiding this comment.
Thanks for pursuing the best possible UX ✨
I'd love to see a screenshot before/after showing the behaviour on submenus.
It looks great when there's only a single level but how does that work when you are trying to edit a Nav item several levels deep?
The original purpose of adding the capture toolbars functionality was to improve UX for nested items so we should test that scenario heavily.
|
Absolutely, sorry for not including that. Here's building submenus with the toolbar absorbed:
Note that the behavior of the appender requiring you to select the parent first is separate, and being looked at in #36720. Here's with the toolbar not absorbed:
|
|
I pushed a tweak to capture the toolbars for submenus. I think it works incredibly well: It feels like the toolbar is exactly where it should be for that use case. What do you think? Note that this means it no longer mitigates #36977, but it looks like the "moving toolbars below" change did not work as well as we had hoped anyway. |
getdave
approved these changes
Dec 1, 2021
There was a problem hiding this comment.
To me this seems logical. Allow the toolbar to move to individual items at the topmost level, but fix it to the top level parent when editing submenus.
My only other question is what happens when you nest submenus? Does it still stick to the topmost parent?
Thanks for working on this.
PS: is this a "bug fix" we want to backport or an enhancement?
Co-authored-by: Dave Smith <[email protected]>
The toolbar still gets captured in the topmost version. I know the GIF goes long, but it shows that use case towards the end, and I think it works rather well honestly. Does it need to land for 5.9? I don't know. I think it's a good and harmless enhancement, but it also fixes #36538, which I honestly feel is a buggy experience. But I also don't have a strong opinion. Taking the long view to where we want to be, this will be a good fix. Will the experience in the mean time be compromised without it? Maybe not, it's always a sum of things. Happy to defer! Thank you for the review and code suggestion! |
|
I say we land it as a "bugfix". The rationale you present is strong. We should make sure the PR description clearly describes which bug it "fixes" though. Also...sorry for not watching the GIF towards the end 😅 |
getdave
added
[Type] Bug
noisysocks
removed
the
Backport to WP 6.7 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Dec 6, 2021
* Navigation: Try removing absorb toolbar prop. * Add capture toolbars to submenus. * Update packages/block-library/src/navigation-submenu/edit.js Co-authored-by: Dave Smith <[email protected]>
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.
Description
Fixes #36538, mitigates #36977 and #36701. But the primary reason for creating this PR was #36701. Props to @getdave for pointing me in the right direction.
The absorb toolbar prop exists for the parent block to absorb the block toolbar of children. This is useful for some blocks that feature very compact or intricate child blocks, or for some use cases such as a sketching block where child blocks are blocks you insert and rotate inside as innerblocks.
It was created for the older version of the navigation block, where the block UI was much heavier to the point that borders were overlaying and creating confusion on their own. With the simpler handling of borders on focus, this is less of a concern with the navigation block than it was at the time, and on the flipside, the change comes with its own tradeoffs:
Due to the balance tipping, I'm currently leaning back towards not absorbing the toolbar, in part also because of the two important bugs that the change mitigates. I also think we could keep a semblance of the absorb behavior, for example absorb the toolbar only for submenu blocks.
Before:
After:
How has this been tested?
Insert a navigation block and observe the toolbar being in spatial context of the child block selected.
Checklist:
*.native.jsfiles for terms that need renaming or removal).