Duplicate LeafMoreMenu into the navigation block and the global sidebar navigation #50489
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: +546 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
jeryj
commented
May 9, 2023
scruffian
requested review from
draganescu,
getdave,
MaggieCabrera,
ajlende and
scruffian
|
Flaky tests detected in 1f5a962. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4949759591
|
|
I moved this to the navigation block, but that breaks it for the browse mode sidebar. 🤔 |
Hrm. It was a good idea, though! Conceptually I like the idea that the menu that's particular to the navigation block, and intends to override the default dropdown menu, should live with that block (or closer to the consumer of the ListView, rather than the ListView itself). If it isn't possible to move it directly to the Navigation block, then would it be worth renaming it from |
scruffian
force-pushed
the
refactor/move-leaf-more-menu
branch
from
d7c55be to
8efb4c6
Compare
scruffian
changed the title
scruffian
force-pushed
the
refactor/move-leaf-more-menu
branch
2 times, most recently
from
1e5e5bd to
4a87a03
Compare
draganescu
reviewed
May 11, 2023
| @@ -34,6 +34,8 @@ const MainContent = ( { | |||
| isNavigationMenuMissing, | |||
| onCreateNew, | |||
| } ) => { | |||
| const { OffCanvasEditor } = unlock( blockEditorPrivateApis ); | |||
draganescu
reviewed
May 11, 2023
| @@ -1,3 +1,5 @@ | |||
| // NOTE: This file is duplciated in packages/block-library/src/navigation/edit/leaf-more-menu.js | |||
There was a problem hiding this comment.
This comment is not needed. They're two different things that just happen to be the same today.
That is not why :) - the reason is because ListView has composable parts and we can compose ListView and LeafMoreMenu depending on what shape we want LeafMoreMenu to be - not just rely on the default block more menu which the leaf shows if not configured. |
MaggieCabrera
approved these changes
May 11, 2023
draganescu
approved these changes
May 11, 2023
There was a problem hiding this comment.
This tests well for me and the code does the job of making the leafmoremenu a component that is composed from the various places that use ListView.
The leaf more menu in the navigation screen of the site editor does not perfectly work. Particularly the add submenu:
does not expand the link with the added submenu item
the added submenu item shows up LinkUI - which is as broken as always in this context
I will approve this anyway as I think it's a good step forward.
… be deleted Move LeafMoreMenu to its own component Move Leaf More Menu to navigation block remove private API Add Leaf More Menu to the browse mode sidebar Add a note to show these files are duplicated
scruffian
force-pushed
the
refactor/move-leaf-more-menu
branch
from
ed6e5da to
1f5a962
Compare
ciampo
reviewed
Jun 13, 2023
There was a problem hiding this comment.
👋 Came across this PR while working on some refactor to DropdownMenu. I noticed that there's quite a lot of duplication between:
- the block settings dropdown
- the leaf more menu in the block library package
- the leaf more menu in the edit site package
Have you considered/planned for some refactoring work to reduce this duplication (especially between the two "leaf more" menus), or do you believe that these dropdown menus are gong to diverge further as time goes on?
| import { BlockTitle, store as blockEditorStore } from '@wordpress/block-editor'; | ||
|
|
||
| const POPOVER_PROPS = { | ||
| className: 'block-editor-block-settings-menu__popover', |
There was a problem hiding this comment.
The styles associated with this class name belong to the block-editor package.
We should avoid re-using class names (especially across packages) as it can lead to unexpected side effects. I suggest we add a new classname specific to this file/package, and (if needed) copy the styles from the block editor over.
The same should be done for the other "leaf more dropdown" component
There was a problem hiding this comment.
After looking more into this, it doesn't look like this class achieved much in terms of styling (the padding override doesn't seem to have effect). I'll also look into other parts of the repo relying on this, and will potentially remove the class altogether
| className="block-editor-block-settings-menu" | ||
| popoverProps={ POPOVER_PROPS } | ||
| noIcons | ||
| { ...props } |
There was a problem hiding this comment.
There are quite a few properties that are being passed to DropdownMenu via { ...props } that are not relevant for the component (ie. blocks, expanded, expandedState, clientIds, ...).
This pattern has the potential to causing unwanted side effects, especially hard to track when performing wide refactors. We should be trying to restructure props more explicitly, especially when we know exactly where those props need to be used / forwarded.
I think they will, but we aren't sure yet... |
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.
What?
Moves
<LeafMoreMenu />into its two new components so the<OffCanvasEditor />can be deleted.Why?
So
<OffCanvasEditor />can be deleted.How?
Duplicate the component into two new ones and update the dependencies.
Testing Instructions