Navigation: Try always showing appender even when child is selected. #37637
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: +635 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
Thanks so much for taking into consideration the feedback a few of us gave. I'm always so impressed by how you dig into the details, surface, and find sound solutions. In testing this out, this feels a lot better and smoother. Simply having an easy to see + button in the navigation block experience goes a long way. Without it, it feels clunky or like something is broken. For some reason, this feeling never has come up with the Button or Social Icons block and I can't put my finger on why. I think if I had to guess, it's because the Navigation block feels more like a "builder" or container compared to the button or social icons. I expect to have more options to add since there are more options to build out with it. |
jasmussen
force-pushed
the
try/navigation-always-show-appender
branch
from
0f1455e
to
3e1715a
Compare
|
I've rebased this one just to shepard it along, and it still works as originally outlined, showing the parent add button even when a child is selected: My original concerns remain, this is bespoke software for the navigation block. That should generally be the exception, as it fragments the experience and might paint over issues that should be fixed at the source. At the same time, it's small, and good to have around in case this issue resurfaces. I'll mark this one as ready for review to gather some more thoughts. |
tellthemachines
approved these changes
Feb 2, 2022
There was a problem hiding this comment.
This works really well, and has the added bonus of making it much easier to add nav items with just the keyboard, which is currently really painful to do.
It is a hacky approach, but would also be welcome in Social Icons where it's seemingly impossible to add children with keyboard only. Perhaps something like this could be implemented as a toggle on the default appender?
| @@ -39,7 +40,6 @@ const LAYOUT = { | |||
| export default function NavigationInnerBlocks( { | |||
| isVisible, | |||
| clientId, | |||
| appender: CustomAppender, | |||
There was a problem hiding this comment.
I'm not sure if this is still being used by the navigation screen; might be worth preserving the option of passing in a custom appender.
There was a problem hiding this comment.
I'm not sure how to best address this feedback item, but feel free to push to the branch if you like.
There was a problem hiding this comment.
I played around with it and realised the previous code wasn't working on the nav editor (probably due to the changes we made to the block late last year) so I just removed the prop being passed to NavigationInnerBlocks altogether.
jasmussen
force-pushed
the
try/navigation-always-show-appender
branch
from
3e1715a
to
2e669b9
Compare
For social links specifically, right now it's using a custom URL dialog, and could definitely benefit from a bigger refactor, not just to solve the keyboard issue but to improve the whole flow.
I'd like to see this. It's the hackiness and inconsistency about this implementation that worries me the most. Longer term it still seems possible for the sibling inserter to be improved to a point where it can entirely replace the black plus. |
Agreed, let's get this in as a quick fix and look at a better long term solution later. The sibling inserter needs work on keyboard a11y too. |
|
Thank you for the help! I took it for a spin and things are still working as expected. In context of the discussion, I'd be happy to try this, so let's land this one and follow up. |
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 #37572. This PR is a draft as it needs a good sanity check, parts of the code are loaned from a similar change we made to submenus in #36720.
When manually building out a navigation block, the effort in #34041 is geared towards quickly adding items and choosing their destination. A separate effort in #36605 was merged to ensure we avoid layout shifts in the site editor. Those layout shifts occurred because the default appender would appear in the content and take up space when selected.
Both of those efforts are important, improvements are planned for both. But in the mean time, the experience of building out a navigation block from scratch has taken a step backwards, needing for the navigation container block itself to be selected to surface the appender:
While there are mitigations (just open the top left inserter and add as many items as you want) it's not an ideal interim state. This PR makes a change to the default appender behavior, and shows it even when a child block is selected:
This change has its own tradeoffs:
Ultimately these interfaces all need more work. For example the need to manually build out a menu can be reduced by loading pages by default (see also #36667), or by enabling navigation to be edited in isolation just like template parts. The sibling inserter (the blue line) could also potentially replace the little black in-canvas plus entirely, by enabling it to work before first block or after the last block:
But those aren't near-term changes. In that light, this PR should be thought of (and comments reflect that) as a temporary solution: a bandaid we should work to remove. What do you think?
How has this been tested?
Insert a navigation block, start empty, then add multiple top level navigation items.
Test submenus and all other options for good measure.
Checklist:
*.native.jsfiles for terms that need renaming or removal).