-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
List View: Allow the component to show a custom "more" menu. #48097
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a good idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
e956a19
to
76679ff
Compare
One thing I thought about was that we could provide a prop called |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks that's useful context, pun not intended!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't an Object
.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
496afa5
to
c1db948
Compare
I think I have addressed all the feedback on this now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks for addressing all the feedback.
I pushed a small commit to update the JSDoc types. Hope you don't mind.
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.