Try a fixed toolbar in the navigation page #21340
Conversation
talldan
added
[Type] Enhancement
talldan
requested review from
draganescu,
adamziel,
karmatosed,
mtias,
noisysocks and
tellthemachines
|
Size Change: -49.1 kB (5%) ✅ Total Size: 840 kB
ℹ️ View Unchanged
|
|
What's happening with the issue @draganescu spotted is that when the menu is changed, the blocks are replaced using the I tried a fix to the block editor store in #21371, but I don't think it's going to be straightforward, so I've pushed an intermediate fix in one of the last commits that calls We might actually want to select the root navigation block when the menu is changed rather than clearing selection. |
talldan
force-pushed
the
try/navigation-page-fixed-toolbar
branch
from
2701495 to
4aa01fb
Compare
|
This makes sense to me. |
| } | ||
| > | ||
| <PanelBody title={ __( 'Navigation menu' ) }> | ||
| <NavigableToolbar |
There was a problem hiding this comment.
Wouldn't it be fine to just render the toolbar slot in a fixed position?
There was a problem hiding this comment.
Using the BlockToolbar component mirrors the way the fixed toolbar is implemented in the post editor. My thoughts were that this is preferable as the movers are implemented differently in BlockToolbar component—they're inline buttons. Whereas with the Slot, the movers reveal and have a drag handle, which doesn't seem like the right behaviour for an inline toolbar.
|
Was testing this and ran into an issue where pressing Esc to go into Navigation mode causes Seems to point to (this line)[https://github.com/WordPress/gutenberg/blob/e8f4f37d5c4e45ce545ddf55e387100ae996a15e/packages/components/src/popover/index.js#L330-L331] |
talldan
force-pushed
the
try/navigation-page-fixed-toolbar
branch
from
5e2ef89 to
ee226a0
Compare
|
@tellthemachines Thanks for spotting that. I fixed it in the last commit. Navigation mode depends on
|
Can we hide the toolbar when in Navigation mode? It probably shouldn't be shown unless in Edit mode and a block is selected. |
Yep, that might be a good option. Will work on that 👍 |
talldan
force-pushed
the
try/navigation-page-fixed-toolbar
branch
from
b28628e to
d8be3a9
Compare
There was a problem hiding this comment.
I can still cause the page to crash in a similar way to what @draganescu found: after making changes to a menu item and clicking "Save navigation", the "More options" button remains visible in the toolbar:
| <Panel> | ||
| <PanelBody | ||
| title={ __( 'Navigation structure' ) } | ||
| initialOpen={ isLargeViewport } |
There was a problem hiding this comment.
Not sure what's wrong, but this is no longer applying? The panel is always closed, whatever the screen size.
There was a problem hiding this comment.
Hmm, yeah, unusual. Not sure why this happens.
I moved the useViewportMatch call back to its original location in a796f3c and it works again now.
@tellthemachines thanks for spotting that! It's the same issue as before, There seem to be a few issues that I encountered to document 😵:
aff58f7 and a56ef6e solve the problem for temporarily. |
Description
There are a couple of issues with the toolbar in the navigation page at the moment:
This PR tries using a fixed top toolbar to solve those issues. This toolbar might also become useful for other functionality (undo, redo) on this page in the future.
The negatives of this are that if someone is creating a long nav menu they might have to scroll up to access the toolbar. The empty menu also looks unusual when no block is selected.
How has this been tested?
Screenshots
Before
Initially I tried adding some empty space for the toolbar, but it results in this empty space when typing
This PR, using a fixed toolbar, it doesn't hide when typing (but is empty when no block is selected)
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: