List View: Allow the component to show a custom "more" menu. #48097
Conversation
scruffian
added
the
[Feature] List View
packages/edit-site/src/components/secondary-sidebar/list-view-sidebar.js
Outdated
Show resolved
Hide resolved
|
Size Change: +1.23 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4a6cc2d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4540405026
|
|
I updated the approach here based on this comment. I didn't see any value in passing the more menu in a "blockControls" prop - How would we know how to handle each of the different controls? Maybe I misunderstood... Now the list view has no "more" menu by default. |
| @@ -117,6 +117,7 @@ export { | |||
| useBlockSelectionClearer as __unstableUseBlockSelectionClearer, | |||
| } from './block-selection-clearer'; | |||
| export { default as BlockSettingsMenu } from './block-settings-menu'; | |||
| export { default as BlockSettingsDropdown } from './block-settings-menu/block-settings-dropdown.js'; | |||
There was a problem hiding this comment.
Is this a good idea?
There was a problem hiding this comment.
How about instead making BlockSettingsDropdown the default value of the new prop.
That way it doesn't need to be imported by the edit-site package.
|
Another option here could be to use slots and fills... |
packages/edit-site/src/components/secondary-sidebar/list-view-sidebar.js
Outdated
Show resolved
Hide resolved
I'd love that, but I don't think a slot/fill will be possible as I think a slot can only be rendered in one place, whereas ListView would require slots for every row. I may be wrong though.
It'd be a render callback function that's executed within each This did make a lot more sense when the OffCanvasEditor had the extra 'Edit' button. Now that's removed I can still see why it looks like it'd be better to add a |
scruffian
force-pushed
the
update/list-view-more-menu
branch
from
e956a19
to
76679ff
Compare
|
One thing I thought about was that we could provide a prop called |
There was a problem hiding this comment.
IMO we should be trying to build solutions that solve the current problem rather than trying to anticipate things that we may not need.
Developer experience and tech debt should also be considered as current problems. I think there have been enough pain points with list view in the past and enough open feature requests to consider changing how it works. It's not so much "anticipating thing we may not need", but looking at where difficulties have been in the past every time something needs to be added or taken away from List View and deducing that this problem is not going away in the future.
As this is a fairly small change, I think it's ok to merge it. I think the best option would be to make it private so that there is still the possibility of refactoring list view in the future.
| @@ -59,7 +62,11 @@ export default function ListViewSidebar() { | |||
| focusOnMountRef, | |||
| ] ) } | |||
| > | |||
| <ListView /> | |||
| <ListView | |||
| MoreMenuComponent={ ( props ) => ( | |||
There was a problem hiding this comment.
This should be simplified to MoreMenuComponent={ BlockSettingsDropdown }, as the wrapping function doesn't do anything.
MoreMenuComponent={ () => <BlockSettingsDropdown /> } is essentially declaring a component at render time, which isn't something you should do in React (it could be the cause of some failing tests).
The second unwanted side-effect is that this wrapping function has a different reference on every render, and so it causes ListViewContext to have an updated value on every render.
The alternative pattern is to treat this as a render function, but I don't know if it's needed, that may cause other issues.
| __experimentalSelectBlock={ updateSelection } | ||
| /> | ||
| ) } | ||
| { MoreMenuComponent && |
There was a problem hiding this comment.
The boolean logic should be moved up to where showBlockActions is.
At the moment if MoreMenuComponent isn't defined, an empty TreeGridCell will still be rendered.
| expandedState, | ||
| expand, | ||
| collapse, | ||
| MoreMenuComponent, |
There was a problem hiding this comment.
I don't love the fact that this context value is being used for so much, as it has performance implications.
The more values it tracks, the more value changes and the more subscribers it's likely to have, which means more unnecessary renders (and List View is a very performance sensitive component).
Realistically, this one is unlikely to cause problems if the issues mentioned in other comments are fixed, so I don't mind if it's kept as is, but it is something that should be tackled.
(I've left this PR feedback before, and yet it still keeps getting bigger!)
There was a problem hiding this comment.
Thanks that's useful context, pun not intended!
There was a problem hiding this comment.
What's the alternative to putting it in context? Prop drilling I assume. Is there any other alternative?
There was a problem hiding this comment.
It can be split into separate context providers, something like:
- ListViewExpandedContext
- ListViewDragAndDropContext
- ListViewOptionsContext
And isMounted can probably removed if we can figure out what it was for.
So many context providers does make the code a bit uglier, but I think that's a fact of life right now with React.
| blocks, | ||
| showBlockMovers = false, | ||
| isExpanded = false, | ||
| MoreMenuComponent, |
There was a problem hiding this comment.
An upper-case first letter for a prop doesn't seem right, I think the pattern is usually to keep the prop with a lower case letter and then alias it to seem more component-like:
| MoreMenuComponent, | |
| moreMenu: MoreMenuComponent, |
Also, I notice it's called MoreMenu here, but it's called BlockSettingsMenu everywhere else in the codebase.
It'd be good to make this private as well.
| * @param {Array} props.blocks Custom subset of block client IDs to be used instead of the default hierarchy. | ||
| * @param {boolean} props.showBlockMovers Flag to enable block movers | ||
| * @param {boolean} props.isExpanded Flag to determine whether nested levels are expanded by default. | ||
| * @param {Object} props.MoreMenuComponent Optional more menu substitution. |
| @@ -117,6 +117,7 @@ export { | |||
| useBlockSelectionClearer as __unstableUseBlockSelectionClearer, | |||
| } from './block-selection-clearer'; | |||
| export { default as BlockSettingsMenu } from './block-settings-menu'; | |||
| export { default as BlockSettingsDropdown } from './block-settings-menu/block-settings-dropdown.js'; | |||
There was a problem hiding this comment.
How about instead making BlockSettingsDropdown the default value of the new prop.
That way it doesn't need to be imported by the edit-site package.
…sidebar.js Co-authored-by: Alex Lende <alex@lende.xyz>
scruffian
force-pushed
the
update/list-view-more-menu
branch
from
496afa5
to
c1db948
Compare
|
I think I have addressed all the feedback on this now. |
Not at all, thank you! |
What?
This allows the ListView to accept a custom "more" menu component. Closes #46991
Why?
When building the Navigation List View, we discovered a need for different "more" menus in different contexts. This will allow us to reuse the list view component in the Navigation block rather than having a copy of the ListView.
How?
Adds a new prop to ListView which specifies the custom "more" menu.
Testing Instructions
Testing Instructions for Keyboard
As above.
Note
We will need to add the "more" menu back to the post editor before we merge this.