Ellipsis menu in block navigator #22427
Conversation
adamziel
added
[Block] Navigation
|
Size Change: +429 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
adamziel
force-pushed
the
add/ellipsis-menu
branch
from
cdc90d1 to
691c486
Compare
|
Right now this uses |
noisysocks
added
[Feature] Navigation Screen
[Type] Enhancement
There was a problem hiding this comment.
Nice work!
-
The More options button overflows past the Navigation structure panel on my 1440px wide viewport.
-
The app crashes when I click on an item's text and then click on the More options button.
-
Should we add the ellipsis to the Block Navigator in the post editor?
| [ | ||
| __experimentalWithBlockNavigationSlots, | ||
| __experimentalWithEllipsisMenu, | ||
| ] |
There was a problem hiding this comment.
Why are we using context for these two settings? Would prop-passing be simpler? 🙂
There was a problem hiding this comment.
Normally I'd say it would be, but this is a quite entangled network of components with a recursive structure - we'd have to pass these two props through ~5 layers of components. I think the context makes it simpler in this case.
There was a problem hiding this comment.
I also think context saving some repeated props works.
packages/edit-navigation/src/components/menu-editor/navigation-structure-panel.js
Outdated
Show resolved
Hide resolved
|
If you use the vertical ellipsis, it doesn't take up as much space. The smallest tap target we accept is 24x24, so there's a great deal of horizontal space to find there. With regards to the mover control, I'm unsure if that's part of this PR or not, if it isn't we can find a way to address this separately. The movers need to sync up with what we're going to explore next for block movers (this: #21935 (comment)) — so a 48px tall unit that uses the same icons. |
adamziel
requested review from
ajitbohra,
jorgefilipecosta and
Soean
as code owners
adamziel
force-pushed
the
add/ellipsis-menu
branch
from
f1ee8c2 to
30cdfaa
Compare
|
@noisysocks @jasmussen all good notes - thank you so much! I addressed your feedback and this PR is ready for a re-review now :-) |
@jasmussen it's not a part of this PR - these movers are already there. |
…-structure-panel.js Co-authored-by: Robert Anderson <[email protected]>
| __experimentalWithEllipsisMenu: withEllipsisMenu, | ||
| __experimentalWithEllipsisMenuMinLevel: ellipsisMenuMinLevel, |
There was a problem hiding this comment.
Small thing, but I think we shouldn't use the term EllipsisMenu for the menu.
Elsewhere in the code it's called BlockSettings, so I think it'd be good to stay consistent with that.
There was a problem hiding this comment.
I have been pondering this exact issue. The two are not exactly the same. BlockSettings offers certain set of options:
And the ellipsis menu is supposed to include everything from BlockSettings PLUS everything from the toolbar as well:
While I don't love the name EllipsisMenu, I am also not convinced that using the name BlockSettings for both menus is the right thing to do here. Let's discuss that more on #22462 - we may need to rethink the naming strategy a little bit more as we also have BlockControls and then UniversalBlockControl. It's just too many similar names in circulation :-)
There was a problem hiding this comment.
In either case EllipsisMenu is bad because it references the icon we use for the menu. What if we change that icon to a circle?
If we want a menu that includes the BlockControls and the BlockSettings which is used by a Block Navigator Item, we'd wrap the two (for reuse if that is the case) in a thing that references the functionality, for example BlockNavigatorItemSettings.
Also, since BlockSettings has Slots we can use these to extend that in the BlockNavigatorItemSettings. If we want slots to be available for actions/ items specific to to BlockNavigatorItem then we'd add slots to BlockNavigatorItemSettings.
There was a problem hiding this comment.
@draganescu as I mentioned I agree we shouldn't call it EllipsisMenu - I was simply looking for a better option :-) BlockNavigatorItemSettings sounds pretty good so I went with it in #22463
There was a problem hiding this comment.
Ah, I don't think BlockNavigatorItemSettings is right either. The Navigator is called Navigation internally and the Item is called Block, so I reckon BlockNavigationBlockSettings is perhaps the right way to go.
I know it's weirdly repetitive, but it's just a component name, and it makes it pretty clear where it lives and that it's a version of the BlockSettings.
There was a problem hiding this comment.
@adamziel @draganescu @talldan I'm confused here, why shouldn't the two menus be the same?
There was a problem hiding this comment.
Because in the Navigator the Items might have additional options that don't make sense in the block's menu. I don't know if that is the case @mtias. One example is add submenu, but that should sit fine with the block as well, why not?
There was a problem hiding this comment.
Yes, they seem to serve conceptually the exact same purpose. If there are certain features that would not make sense on one or the other, we should probably handle it through configuration / context.
Description
This is a first attempt at adding an ellipsis menu to block navigation. The menu only offers a few basic options in this initial PR.
A follow-up PR will bring in more menu items. I propose merging/reviewing these PRs in the following order:
Related issue: #22089
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: