Navigation: add dedicated sidebar for managing layout of navigation menus #39290
Conversation
gwwar
requested review from
ntwb,
nerrad,
ajitbohra,
ellatrix and
tellthemachines
as code owners
|
Size Change: +1.27 kB (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
Thanks for putting this together |
| @@ -140,6 +142,11 @@ export default function BlockToolbar( { hideDragHandle } ) { | |||
| group="other" | |||
| className="block-editor-block-toolbar__slot" | |||
| /> | |||
| <__unstableBlockNameContext.Provider | |||
There was a problem hiding this comment.
This is used to provide a toggle to open the Navigation sidebar for the currently selected Navigation block.
There was a problem hiding this comment.
Added a picture for folks too. In trunk clicking this opens a list-view modal, but on this branch it should open the new sidebar.
There was a problem hiding this comment.
I took this for a spin. It's a really great start and I can see it provides some benefits as a way to manage a Navigation Menu outside of the Navigation block (which can have its limitations).
Nonetheless there are some rough edges and pain points that we need to either
- fix in this PR
- note down as "known limitations" and track in follow up Issues
This is not intended to sound overly critical. I fully acknowledge that this isn't the "finished article" and we're still iterating on the design and UX of this approach. Moreover, I will likely be working on this PR so it's in my interests to be thorough in exposing problems 😄
Here are my thoughts:
UX problems
- the requirement to side scroll to see the ellipsis menu on long menu item names is suboptimal
- the loading animation doesn't marry well with the positioning of the menu when it renders
- we might want to avoid rendering the menu with all levels fully expanded. This adds a lot of visual noise and makes the menu more difficult to explore for simple operations.
- when toggled open/closed the sidebar seems to have trouble restoring the menu you were last editing
https://user-images.githubusercontent.com/444434/157650683-436236d6-0678-41b0-90bd-ca026f4d3aeb.mov - it's possible to toggle block settings from the menu item. When you do this it toggles the block inspector for the block that is currently selected within the editor canvas.
https://user-images.githubusercontent.com/444434/157651417-036f3307-e589-4358-a90d-4f8bd18dd859.mp4 - drag and drop indicators (blue lines) overflow outside of the sidebar
Missing features
These may or may not be within the scope of this PR or indeed the feature more widely. But we should acknowledge them:
- inability to add new menu items - tricky due to inability to edit text...etc
- inability to create new menus - this is important for "safe exploration" (the ability to create menus without having to render them to a post via the Nav block).
Summary
I think this is a great start and I appreciate all the effort that's gone into it 👏
The implementation aside however, even if it were polished I cannot see it solving all the user requirements for managing menus outside of the block. We are missing
- ability to create new menus
- ability to directly manipulate the menu item text content
- ability to manipulate the menu item block settings
- ability to access the raw inner blocks of the Navigation Menu itself
Therefore I wonder whether we should restyle this sidebar less as a means to manage the content of the menu and instead make it more about managing the structure of a menu. Indeed this may have been the original genesis of the idea as outlined in #36667.
|
Noting some related issues to ListView that can be worked on outside of this PR:
There's some discussion in #34759 for that. Folks prefer collapsed by default, there will need to be a small refactor for component open/closed state in ListView. Folks should also be mindful of making sure windowing working as expected when ListView has many items.
This is a general issue with ListView taking up an arbitrary width based on the current contents. It's more noticeable here since the right sidebars are smaller than the secondary ones. I played around with this a bit in #35605 though I think it's ideal if we can standardize a bit on sidebar widths as well. @jasmussen do you know if there was an existing issue for this one?
We likely only need to fix this if we want this panel to land on the right vs left. Shouldn't be too bad to tidy up. Also note that folks have been wanting more direct dragging in ListView. I recently updated framer motion to v6 which has new drag utilities, so folks should be able to experiment with that again. Previously performance was a blocker for this, but ListView also uses windowing now for larger cases so that shouldn't be as much of an issue. Here's a prototype of what it could look like using framer v4: #34022
🤔 Is that not what the panel currently does? |
skorasaurus
added
the
[Feature] Site Editor
|
I am testing the PR. Clicking the "List view" icon in the toolbar opens panel in the sidebar. I would assume that it would be associated with the Navigation block sidebar settings. Clicking the X closes the right sidebar.
Here it could instead be on top of the panels kinda like clicking the 3 dot contextual Options icon in the top right corner opens on top of the sidebar settings. Here it can remain connected with the Nav block sidebar settings.
Another option. Here is the mockup showing only the one Edit panel. If it can not directly be added to the sidebar panels then one could perhaps have a panel with information and a button to open the Navigation editor.
|
|
I think the intention of this panel is ultimately that it becomes a "site level" panel as outlined in #36667. Eventually we'll be able to manage all navigation blocks here, and create new ones. This PR is just a first step towards that goal, and although neither of those things are yet possible we should be looking to move in that direction. It is easier to get there in small increments, by only changing one thing at at time. I think the next step will probably be to make this panel dark and move it, and the Styles sidebar to the left of the window, so I'd rather keep the behaviour of the sidebar as is for now and revisit it in the next PR. |
|
Thank you for the feedback everyone. I agree we ought to keep the scope of this PR narrow and iterate in future PRs. I'm going to look at ironing out bugs and then addressing some of the points that have been raised. |
This comment was marked as outdated.
This comment was marked as outdated.
getdave
added
Needs Design Feedback
|
It would very valuable to gain some design input here. Note: we're specifically trying to limit the scope of this PR to encompass adding a place to modify your Navigation Menus only. Items where I think design could add most value:
I will update the PR description with that latest screenshots and information. |
|
For a first attempt, I don't think the user experience is too far off. I don't recommend trying to ship the block movers or any other new List View features in this PR, maybe a follow-up? It doesn't all have to be done in one go. They've been inactive for probably about two years, I think they'll need some refinement, accessibility testing and e2e tests. I think it'd be good to consider moving this feature to the left of the screen before merging though. At the moment a user can have two List Views open, which is confusing. I think this one should just replace the global List View when it's selected from the nav block. The designs all show the feature on the left, so I think it's best to start with that so that the ergonomics are correct (e.g. Block Settings can be toggled open). The sidebar on the right is also a bit narrower, so not ideal for this feature. |
|
Thanks for taking time to feedback here @talldan.
I don't think block movers are a new feature of list view. They come "out of the box" so to speak. What would you suggest? Disabling them? I could remove other features I've added to ListView into smaller follow ups. That should keep things simple as long as it doesn't muddy the review process. I agree that shipping in smaller increments is best. |
Oh right. They've never been shipped as an active feature. I guess they're automatically enabled in the List View component? Maybe they're included here accidentally. I think they can be turned on or off via a |
|
Currently this feature is not very exposed to users - let's use that to our advantage. We could look to simplify this PR by removing non-essential features (moving those to separate PRs) and shipping "as is". We can then follow up to:
@talldan @scruffian what do you think? |
|
Thanks for taking a stab at a minimal first version! Here's what I see in quick GIF form: A few thoughts. Firstly that it seems fine to start the pinned item on the right, even if eventually we'll likely move it left as part of #36667. Perhaps an artifact of my local env. being sheparded along without a fresh restart in a while, but I can't find in the dropdown list the particular menu used on my site. "Header Navigation" as the chosen menu doesn't show up in the list:
Note that if I choose a menu in the canvas which also shows up on the right, the canvas DOES update to reflect any changes I make dragging things around. "Navigation" seems like a good simple name for the panel. The existance of this panel itself seems to suggest a good next step would be to explore: what happens if there are zero navigation menus created saved? Can we show the structure of the site itself (using page metadata such as parent and sort order), and then save a menu as soon as a customization is made? If I only ever have a single menu saved, we don't need to show the dropdown:
If I select a navigation block in the canvas, and I have multiple menus saved, should the panel update to pick the selected menu? What do you think? Nice work! |
I think that makes sense. I personally feel this PR could go much smaller, as I'm not sure all the features will make sense to an end user. As an example:
I think the difficult part is that right now the feature feels very connected to the editor canvas, but the end goal is for it to represent and be used to build a more global site structure. Some of these features are maybe being introduced too early (the menu dropdown being the main one). |
This is now handled in #39494
getdave
force-pushed
the
add/navigation-sidebar
branch
from
4c75946 to
9a59e6e
Compare
|
@tellthemachines Thank you so much for your review. As this PR is growing ever larger I'm going to look to merge it and then address these points as follow ups. They are important and I will raise Issues for each now and ensure they are addressed as may next tasks.
Ok let's rename that. Tracked in: #39800
Hmmm. Let me test the other sidebars and see what happens. Should be possible to auto-focus inside the sidebar. Tracked in: #39801
Is this something we also experience with ListView in other contexts (as in the primary List View)? I'll take a look. Tracked in #39802.
Agreed. Great spot. Tracked in #39803. |
|
Thanks for following up with those issues @getdave !
I'm not sure as the primary List View doesn't have the movers. It occurred to me that this could be related to keyboard improvements in #38358, which changed the behaviour of Left/Right arrows to allow expanding and collapsing rows better. |
| <ListView | ||
| id={ id } | ||
| showNestedBlocks | ||
| showBlockMovers |
There was a problem hiding this comment.
|
For accessibility feedback, @tellthemachines covered everything I was going to mention. The whole thing is really not keyboard accessible right now. Is there a tracking issue or new PR yet? |
Yes I've listed them out here. |
|
Switching menus does not affect the current selected Header menu. |
That's not a featured we optimised for. Interesting that you expected that to happen. To me this whole thing shows that we need to "divorce" the Navigation sidebar from the concept of the editor canvas. |
|
There's a design outlined in #36667 (comment) which aims to make it clearer that it is a global interface rather than one contextual to the content like the block inspector is. |
What?
This PR adds a new navigation sidebar in the site editor. This sidebar replaces the list view modal, and allows one to see the contents of navigation menus even if they aren't in the current template.
Screen.Capture.on.2022-03-15.at.14-09-57.mov
Why?
Part of explorations for #36667.
This PR pulls out a more pragmatic piece of #38600 out to focus specifically on a Navigation sidebar.
We may find that it's still a little early to display a new top level panel while we iterate on UX options, so let me know if folks would like to add a feature flag.@getdave: I suggest we ship and iterate based on feedback.How?
A couple of key points in this PR:
__experimentalGetGlobalBlocksByNameallows us to display the correct current menu when the panel is open.BlockEditorProvidergives us the editing and save functionality we mostly expect, however there are side-effects that occur in the rendering of BlockList. This means that we need to think through how to provide information like allowed block information (which blocks may be inserted into another), when BlockList is not rendered (as is the case for ListView). I've added a temporary workaround by dispatching this information manually. Folks should discuss if this temporary patch is worthwhile, or if we should think of a better solution with some refactoring of block list. Note that the current solution is fragile and will drift, but it does fix dragging and has a relatively small surface area.List View has gained new properties to allow all levels to be collapsed by default.(now extracted into Add prop to control default expand/collapse state of ListView nodes #39486)Todo
DisableOut of scope for this PR.Show more settingsoption under Block Actions - doesn't work unless Nav block is selected in editor canvas.ListViewto a separate PR - now done in:Testing Instructions
All of the below should be tested in the Site Editor.
Nav block menu to sidebar association
The intent is to verify that the menu matches that associated with the selected Nav block:
Default Navigation Menu selection based on Template
Default Navigation Menu selection based on available Nav Menus
wp_navigationreturned by the REST API.Modifying Menus items
Misc.