-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
ToolsPanel
: refactor Storybook examples to TypeScript, fix types inconsistencies
#47944
Conversation
Size Change: +14 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
Flaky tests detected in b4ec3017034b890d99e6873d9297b46b94161606. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4161240814
|
label="Tools Panel (default example)" | ||
resetAll={ resetAll } | ||
> | ||
<ToolsPanel { ...props } resetAll={ resetAll }> |
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.
- Making sure that all
props
are passed toToolsPanel
. Thelabel
prop has been moved to theStory.args
object, instead of being hardcoded in the render function. - The
resetAll
prop is also passed (see a few lines above in the localresetAll
function), so that Storybook can correctly fire an action (in the actions tab) when the callback is invoked.
These changes have been applied to all stories.
// `key` property here is used as a hack to force `ToolsPanel` to re-render | ||
// See https://github.com/WordPress/gutenberg/pull/38262/files#r793422991 |
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 is hacky, but not really in the scope of this PR to change. I just added a comment to add more context around it
|
||
export const WithOptionalItemsPlusIcon = ( { isShownByDefault } ) => { |
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.
Since isShownByDefault
is not a prop of ToolsPanel
, in order to adhere to how controls are supposed to work I decided to move it inside the render function as internal state (isFirstToolsPanelItemShownByDefault
). Its value can still be changed interactively, thanks to a button
that I added to the story
}; | ||
|
||
const { Fill: ToolsPanelItems, Slot } = createSlotFill( 'ToolsPanelSlot' ); | ||
const panelId = 'unique-tools-panel-id'; |
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.
Moved to be part of each story's args
object
// TODO: the `ResetFilter` type doesn't specify any attributes | ||
// and doesn't return any objects | ||
// @ts-ignore |
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 is something that I wasn't able to fix on my own. According to the type defs, ResetAllFilter
is of type () => void;
, and therefore is not supposed to accept arguments.
We have two alternatives here:
- either change the
ResetAllFilter
type to accept arguments - or change the next line to
...resetFilter()
This same issue happens a few times across the file
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 for catching this one 👍
In terms of which option, I believe the type is incorrect as prior uses of the resetAllFilter
are to pass through the current data so each filter in turn was aware of the latest changes and wouldn't override updates from previous filters.
If I recall correctly, the color supports panel uses a different batch approach that might allow the second option you presented.
Given there might already be external use of the ToolsPanel, I think I'd still lean towards fixing the type to match existing uses. Also, the ToolsPanel wasn't typed initially and was converted to TypeScript after the fact, which might further point to the types being incorrect.
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 think I'd still lean towards fixing the type to match existing uses
Ok! I tried to find a good way to describe the ResetAllFIlter
type , but the fact that it could literally receive any data as an argument A(or undefined), and return any data (or void
) made me lean towards using ( attributes?: any ) => any
(364fb87)
(@mirka or @tyxla , do you have any suggestions on how to type this callback more strictly ?)
I've also updated the README in both ToolsPanel
and ToolsPanelItem
to reflect those changes (08e55bc)
@aaronrobertshaw , what do you think? If this looks good, I'll let you decide what pattern we should advocate for in example snippets (and potentially change usages in the repo to follow that style too).
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 do you think?
I think this is still an improvement over the incorrect type not matching the callback's intended use.
Unfortunately, I'm short on ideas for typing it more strictly. I'd suggest the general expectation by consumers would be that the same type would be maintained as the data gets passed through each resetAllFilter
. I don't know how we'd achieve that, though.
I'll let you decide what pattern we should advocate for in example snippets (and potentially change usages in the repo to follow that style too).
We have two use cases around resetting values, and only one needs the reset all filters.
- When the ToolsPanel is created, it has a fixed set of child
ToolsPanelItems
, which aren't rendered via slot fills into the panel. This means theToolsPanel
can know what needs to be reset or executed for theresetAll
function and can avoid individual items needing to defineresetAllFilter
. The best examples here are the Global Styles panels in the site editor e.g. Dimensions Panel. - The
ToolsPanel
renders a slot that may contain an unknown number of childToolsPanelItems
. In this case, the consumer of theToolsPanel
needs to rely on eachToolsPanelItem
specifying how to effect its reset via theresetAllFilter
callback. The primary examples of this are the block support panels in the block editor.
Outside of our editors and their block support panels, I believe the first use case above would be the most common. We'd probably be best served to lean towards demonstrating it in our JSDoc example snippets. The Storybook is there to demonstrate the more edge case use of SlotFills
with the ToolsPanel
.
I'll push a commit shortly that might form the basis for our example snippet.
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 might also be worth noting that the passing of the attributes
arg to the resetAllFilter
only came about due to needing to reset only specific properties within nested objects, e.g. only radius, and not color, width, and style, within a style.border
attribute.
Keeping any example use of the ToolsPanel
simple sounds like a win. If a consumer's use case is more complicated the extra Storybook examples provide direction.
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.
Agreed, let's showcase the simple usage (ie. without resetAllFilter
) in our example snippets
hasInnerWrapper={ true } | ||
shouldRenderPlaceholderItems={ true } | ||
__experimentalFirstVisibleItemClass="first" | ||
__experimentalLastVisibleItemClass="last" |
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.
Moved these from being hardcoded values, to being parameters (passed via ToolsPanelWithItemGroupSlot.args
).
I then had to add prop types for ToolsPanelItems.Slot
, although this is still quite messy due to the mixing of ToolsPanel
-specific props with other generic (any
) props that get forwarded to Slot
packages/components/src/tools-panel/stories/utils/tools-panel-with-item-group-slot.tsx
Outdated
Show resolved
Hide resolved
@@ -112,23 +135,27 @@ export const _default = () => { | |||
</PanelWrapperView> | |||
); | |||
}; | |||
Default.args = { | |||
label: 'Tools Panel (default example)', |
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.
Ideally we should pass children
to ToolsPanel
via the children
prop in the args
object. But given how intertwined ToolsPanel
and ToolsPanelItem
components are in these stories, for now I've decided to keep them as they were
Hey @aaronrobertshaw , could you take a good look at this PR and let me know if these changes make sense, in the context of I left a detailed list of what's in this PR in the description above. I've also left inline comments — some of them go in details about the changes that I've applied, but others are pointing at parts of the PR that are still incomplete. If you want to push changes to this branch, feel free to take over the PR and do so! Thank you 🙏 |
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.
Great work here @ciampo ✨
This is looking really good so far. It's great to see the ToolsPanel's kinks being ironed out and the component brought up to scratch.
In general, it tests well for me. The only issues I spotted were a couple of minor copy-and-paste errors in the storybook examples resetAll
functions.
I've also left a couple of replies to inline comments and questions that might allow us to fix the incorrect resetAllFilter
type and cull a very edge case story.
✅ Unit tests pass & no typing errors
✅ No issues found while smoke testing in editors
❓ Only minor reset all issue in storybook examples. Once fixed locally, I couldn't spot any other issues with them.
Hey @aaronrobertshaw , thank you for the review and the extra context! I went ahead and addressed all feedback — all type errors should be gone! The only "pending" item is the Apart from that, it would be great if as part of your final review you could:
Thank you for your patience 🙇 |
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 is testing well for me on all fronts now; nice work 👍
✅ Unit tests pass & no typing errors
✅ No issues found while smoke testing in editors
✅ Storybook examples function as expected
Apart from that, it would be great if as part of your final review you could:
- double check that code, types.ts and READMEs are all in sync
✅ I didn't spot anything amiss between the code, types.ts, and READMEs
double check that code snippets in JDDocs and READMEs reflect a correct usage of the components, especially after the tweaks included in this PR
✅ JSDoc and README examples appear correct to me. It was more the types that were fixed to match existing usage.
double check that usages in the rest of the repo also follow the current types correctly
✅ Manually went through ToolsPanel
uses in Gutenberg and didn't spot any misuse of components or incorrect types.
Before giving my final review, I tweaked the export of the ToolsPanel
and ToolsPanelItem
components from their respective component.tsx
files so that their JSDoc descriptions were included in the Storybook docs. (c0c86d0)
I also added a JSDoc example for the ToolsPanel
. That might be worth double checking before merging. (b4ec301)
From my end though, this looks good to go 🚢
I missed that, thank you for pushing that change!
Looks good too!
Awesome, thank you! I'll plan on opening a PR to refactor unit tests too soon, so that the whole component gets refactored to TypeScript. |
…s` as optional, assign `false` default value
…fault `false` value
b4ec301
to
c6ecc6b
Compare
What?
Refactor
ToolsPanel
Storybook examples to TypeScript, and iron out some type inconsistencies in the process.Why?
Part of #35744
How?
Here's a detailed list of type inconsistencies / fixes:
null
as a possible value forpanelId
on bothToolsPanel
andToolsPanelItem
(as already explained in theToolsPanel
README ontrunk
) (800a5e5)hasInnerWrapper
andshouldRenderPlaceholderItems
props onToolsPanel
as optional (with a default value offalse
). Until now they were marked as required, but they were not passed in many examples (including Storybook) (ebe5493)noop
toresetAllFilter
, since TypeScript would otherwise error about it potentially beingundefined
when used in auseCallback
hook (378ee00)isShownByDefault
prop onToolsPanelItem
as optional with a default value offalse
. Until now it was marked as required, but it was not passed in many examples (including Storybook) 284c5ae)And this is a high-level overview of Storybook changes:
isFirstToolsPanelItemShownByDefault
prop from args to internal state)TODO before merging:
Testing Instructions
The only runtime changes are the assignment of some default values for the props that have been marked as optional.
trunk