ToolsPanel: Try splitting out ToolsPanelDropdownMenu#44284
Closed
noisysocks wants to merge 4 commits intotrunkfrom
Closed
ToolsPanel: Try splitting out ToolsPanelDropdownMenu#44284noisysocks wants to merge 4 commits intotrunkfrom
noisysocks wants to merge 4 commits intotrunkfrom
Conversation
|
Size Change: +55 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
Member
Author
|
@ciampo: Here's a rough attempt at implementing what was discussed in #44180. What do you think? I tried to go further and split out the I'm not 100% sure about the direction. It works and is composition-ey, but, idk... does it make Would a simple prop be better? <ToolsPanel
heading={ <>
<Button icon={ arrowLeft } label="Back" isSmall />
<Heading level={ 2 }>Typography</Heading>
</> }
resetAll={ resetAll }
>
<ToolsPanelItem>...</ToolsPanelItem>
<ToolsPanelItem>...</ToolsPanelItem>
</ToolsPanel>Or, maybe I could use absolute positioning to fake the effect that I'm after in #44180? 😅 |
ciampo
reviewed
Sep 20, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Thank you for trying this out! Code changes are nice and clean, and easy to follow thanks to the detailed PR description
I tried to go further and split out the
GridfromToolsPanelso that we have bothToolsPanel(provides a grid) andToolsPanelProvider(no grid) but had difficulty separating some of theclassNamelogic thatToolsPanelhas.I'm not 100% sure about the direction. It works and is composition-ey, but, idk... does it make
ToolsPaneltoo unopinionated? AToolsPanelshould providePanelbits, no?
I'm not fully convinced either. Let's keep the Grid wrapper as part of ToolsPanel for now, we can always come back to this at a later point.
Would a simple prop be better?
It's definitely a possibility, although I'd personally prefer using children instead of render props, as it feels cleaner to me.
Seeing the route that we're taking here, I think that we should just stop using ToolsPanelHeader internally, and always expect consumers of ToolsPanel to define their own label explicitly (this would also mean removing the label prop from ToolsPanel).
In order to avoid too much disruption during the "transition", we could:
- on this PR:
- keep the newly created
ToolsPanelDropdownMenuas-is on this PR - refactor
ToolsPanelHeaderto behave like currently ontrunk, but refactoring it to useToolsPanelDropdownMenuinternally - refactor most Storybook examples and tests to avoid using the
labelprop - this way, current usages of
ToolsPanelwould keep working as expected
- keep the newly created
- Refactor usages of
ToolsPanelacross the codebase to stop using thelabelprop, and define their own header as part ofchildreninstead (including usingToolsPanelDropdownMenu) - Formally deprecate the
labelprop (outputting a console warning) and scheduling the prop for deletion in 2 WordPress versions time. - When removing the
labelprop, we'll be also able to remove theToolsPanelHeadercomponent altogether.
What do you think? Tagging @aaronrobertshaw and @mirka for thoughts too.
| export function useToolsPanelDropdownMenu( | ||
| props: WordPressComponentProps< ToolsPanelDropdownMenuProps, 'div', false > | ||
| ) { | ||
| const { ...otherProps } = useContextSystem( |
Contributor
There was a problem hiding this comment.
Nit: we don't need the rest operator here
| toggleItem: ( label: string ) => void; | ||
| }; | ||
|
|
||
| export type ToolsPanelHeaderProps = ToolsPanelDropdownMenuProps; |
Contributor
There was a problem hiding this comment.
Nit: we'd probably need to duplicate the type here, since we'd want the description for the label prop to be different for the two components
Member
Author
|
Thanks for your thoughts! API design feels more like art than science so extra opinions are really helpful.
👍 sounds like a plan.
My worry with this is that we're making In other words this: <ToolsPanel resetAll={ resetAll }>
<HStack style={{ gridColumn: 'span 2' }}>
<Heading level={ 2 } style={{ fontSize: 'inherit', fontWeight: 500, lineHeight: 'normal' }}>
Typography
</Heading>
<ToolsPanelDropdownMenu label="Typography settings" />
</HStack>
<ToolsPanelItem>...</ToolsPanelItem>
<ToolsPanelItem>...</ToolsPanelItem>
</ToolsPanel>Becomes this: <ToolsPanel resetAll={ resetAll }>
<ToolsPanelRow>
<HStack>
<ToolsPanelHeading>Typography</ToolsPanelHeading>
<ToolsPanelDropdownMenu label="Typography settings" />
</HStack>
</ToolsPanelRow>
<ToolsPanelItem>...</ToolsPanelItem>
<ToolsPanelItem>...</ToolsPanelItem>
</ToolsPanel> |
Member
Author
|
After some more exploration I realised this won't work for #44180 because what I actually want is for the header to appear completely outside of the <HStack>
<NavigatorBackButton />
<Heading />
<ToolsPanelDropdownMenu />
</HStack>
<ToolsPanel>
<ToolsPanelItem />
<ToolsPanelItem />
</ToolsPanel>I think the only solution to combining the headers in #44180 is to have it so that a consumer can use the To be honest though this is becoming more work than I think it's worth so I'll put this on ice and explore some other options first 🙂 |
Contributor
Yes, that would be a good idea. Basically,
Yeah, the only way there would be to completely destructure
Sure — feel free to ping again if/when you resume work on this, always happy to help |
Member
Author
|
No such thing as a failed experiment 😀 thanks for the thoughts @ciampo |
Contributor
Nonetheless a very interesting API thinking exercise, it may be useful in the future — who knows! Thank you 🙏 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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?
Splits
ToolsPanelDropdownMenuout fromToolsPanel.Why?
So that consumers can implement their own header design while using ToolsPanel. See #44180 for one such example of when we want to do this.
How?
ToolsPanelHeaderis split up intoToolsPanelHeaderandToolsPanelDropdownMenu.resetAllandtoggleItemprops are removed fromToolsPanelHeader. These are now accessed viaToolsPanelContext.labelprop onToolsPanelas optional.GridfromToolsPanel.ToolsPanelwouldn't even really be a panel anymore?ToolsPanel. In particular it usesmenuItemsin order to calculate theclassName.Testing Instructions
Screenshots or screencast