Reuse useToolsPanelDropdownMenuProps util
#64105
Closed
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
SantosGuillamot
added
the
[Type] Code Quality
SantosGuillamot
requested review from
tellthemachines,
ellatrix and
ajitbohra
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This was referenced Jul 30, 2024
|
Size Change: +4 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
cbravobernal
force-pushed
the
try/create-tools-panel-util
branch
from
6180742 to
a82e393
Compare
|
It seems to be working fine, but, as is touching several files, I would be happier if we have a double green check from anyone else in @WordPress/gutenberg-core |
cbravobernal
approved these changes
Aug 21, 2024
This feels like a really adhoc API to export and reuse, if we're using that everywhere why not just bundle it somehow in the component or offer a block-editor specific component that applies it instead. Anyway, I recommend in the future being very intentional when it comes to exporting APIs between packages. Code sharing is very rarely a good argument for exporting APIs. |
|
Also code duplication is ok sometimes. |
ciampo
reviewed
Aug 21, 2024
There was a problem hiding this comment.
I agree with the sentiment that there should be one place where we define settings like the one in this PR (ie. placement and offset for dropdowns used in ToolsPanel in the sidebar) — I was in fact among the people to propose it initially!
I had initially proposed to put this code in @wordpress/interface, since that's where the sidebar (and its width) are defined — that felt like the best place to me, but I understand that we're moving away from that package? In general, I feel like these settings (especially that arbitrary 259) should be as close as possible to where they are applied to the UI.
Having said that, I second Riad's thoughts too — sharing code across packages has its costs — and therefore it should be done only when strictly necessary, because it introduces APIs to maintain, and a dependency in the graph.
Also, do we ever plan on making this API public, or is it going to stay private indefinitely? We should definitely avoid accumulating private APIs when possible.
Comment on lines
+9
to
+15
| ? { | ||
| popoverProps: { | ||
| placement: 'left-start', | ||
| // For non-mobile, inner sidebar width (248px) - button width (24px) - border (1px) + padding (16px) + spacing (20px) | ||
| offset: 259, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Should we extract this object to a constant, to avoid recreating one every time the invoking component renders?
There was a problem hiding this comment.
That question can apply to both objects FWIW: the empty one and the one with popoverProps.
tyxla
reviewed
Aug 21, 2024
There was a problem hiding this comment.
The way we're abstracting this seems a bit off to me:
ToolsPanelcomponent lives in@wordpress/components- The tools panel dropdown menu props are used in
@wordpress/block-editorand@wordpress/block-library - The tools panel dropdown menu props are declared in
@wordpress/block-editor- one of the consumers
To facilitate code sharing, I'd expect we move the prop util in @wordpress/components where the component lives. After all, it seems like we're consistently using the dropdownMenuProps prop with those same values.
If that doesn't make sense because it's not general enough, then it doesn't make sense to expose the util as a private API, and therefore it doesn't make sense to share the code at all (which aligns with @youknowriad's feedback about code duplication being OK above).
This seems a better approach. After feedback received, I will close this PR without merge and make a follow-up. Thanks everyone 👍 |
@tyxla I'm personally not sure about this, since that specific dropdown menu config is something very specific to the block controls sidebar, something that the |
I don't have strong feelings about where it lives, but exposing the props util as private API it doesn't make sense to me. I'd expect we export an instance of the block controls sidebar which always uses those props, and reuse that across the board. |
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?
As discussed here, it seems that each time we are using the ToolsPanel component, we are using the same
useToolsPanelDropdownMenuPropsutil. However, it has been created in different places with exactly the same code.I'm creating this pull request to try to have a unique declaration that is reused across the codebase.
Why?
It can be used in the future for new use cases and we don't need to maintain duplicated code.
How?
I'm creating a new private util in the
@wordpress/block-editorpackage, removed the old declarations, and changed the imports to use this new one.I wasn't sure if the
block-editorpackage was the right place, but util is used in that package and inblock-libraryso far. I'm happy to hear other suggestions.Testing Instructions
Nothing new. Automated tests should pass.