UnitControl: convert Storybook example to TypeScript and Controls
#39320
Conversation
|
Size Change: +15 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
ciampo
added
[Feature] Component System
| <UnitControl | ||
| { ...args } | ||
| value={ value } | ||
| onChange={ ( v, extra ) => { |
There was a problem hiding this comment.
The fact that we have to set the onChange callback from within the template causes Storybook to display an empty callback in the code snippet, wondering if there's a way to avoid this?
There was a problem hiding this comment.
Oh, interesting... and we can't pass it in directly because we still need to call setValue within this component, is that right? For what it is rendering, in this case is it rendering the () => {} because in the UnitControl component it defaults onChange to a noop function? It's probably not terrible for the code snippet to not be showing a useful callback, but I see what you mean — it'd be better if it had a more real-world example usage 🤔
There was a problem hiding this comment.
I went ahead and removed the default noop fallback (both in the code, and as a @default value in the Docs annotations), but that doesn't seem to solve the issue.
It looks like it's showing {() => {}} as if it wasn't able to "introspect" more into the story's code, and actually show the real code for the onChange callback.
I may need to look more into this
There was a problem hiding this comment.
Speaking of event handlers, I was going to ask if the Actions addon would be useful or not. Especially since the work you're doing lately involves a lot of value inspection. (No strong opinion personally, I don't have experience with the addon)
There was a problem hiding this comment.
Going to look more into it, will report back!
There was a problem hiding this comment.
I spent some time looking into why onChange is displayed as () => {}.
- The proper way to fix it would be to change the source type from
dynamic(current behaviour) tocode, as explained in details in the docs - I tried changing this config option in
UnitControl's story, and the code snippet stopped being displayed completely. After some research, it seems like a known bug (Refactor: Split out docs-loader from source-loader storybookjs/storybook#12561) without a fix yet.
I guess we'll have to keep the source as-is, until a fix is released.
Speaking of event handlers, I was going to ask if the Actions addon would be useful or not.
I went ahead and added the addon to the project. I then experimented with it, and decided to only enable it for the onChange callback, since enabling it on the onUnitChange callback was generating a weird output in the code snippet (noRefCheck()):
There was a problem hiding this comment.
Quick update — it looks like the Storybook bug that I had linked previously has been closed and marked as fixed in the release 7, with the @storybook/csf-plugin package released.
Let's see if we need to take action at all, or if updating to Storybook 7 will be enough 🤞
There was a problem hiding this comment.
Thanks for improving these examples @ciampo! It's testing well for me, all the controls appear to be working correctly, and this gives us better automatic documentation in Storybook 👍
Just left a couple of questions: the main one is whether it'd be good to include the label by default since most of the time in real world usage, I think we're usually using labels?
| <UnitControl | ||
| { ...args } | ||
| value={ value } | ||
| onChange={ ( v, extra ) => { |
There was a problem hiding this comment.
Oh, interesting... and we can't pass it in directly because we still need to call setValue within this component, is that right? For what it is rendering, in this case is it rendering the () => {} because in the UnitControl component it defaults onChange to a noop function? It's probably not terrible for the code snippet to not be showing a useful callback, but I see what you mean — it'd be better if it had a more real-world example usage 🤔
| export const Default: ComponentStory< | ||
| typeof UnitControl | ||
| > = DefaultTemplate.bind( {} ); | ||
| Default.args = {}; |
There was a problem hiding this comment.
Should we include a label as part of the default args? I think in most cases of our real world usage of the component, we typically include a label, so it might make a more consistent starting point.
There was a problem hiding this comment.
Fair point, I've added a label to the default args
| @@ -36,7 +36,7 @@ import { useControlledState } from '../utils/hooks'; | |||
| import type { UnitControlProps, UnitControlOnChangeCallback } from './types'; | |||
| import type { StateReducer } from '../input-control/reducer/state'; | |||
|
|
|||
| function UnitControl( | |||
| function UnconnectedUnitControl( | |||
There was a problem hiding this comment.
Would it be helpful to add a comment about what Unconnected means in this context? It took me a little moment to click that this is the raw component, without a forwarded ref, whereas in the other couple of instances where Unconnected is used when naming a component (Divider, and Heading) it's referring to connecting to a context, which is a slightly different concept.
There was a problem hiding this comment.
That's a fair point, I've been a bit lazy in renaming this and went for what we previously did in Divider and Heading, without realizing that UnitControl doesn't actually use contextConnect 😅
For now, I've renamed it to UnforwardedUnitControl. Let me know if you'd prefer alternatives (maybe a more generic BasicUnitControl?)
There was a problem hiding this comment.
UnforwardedUnitControl
Sounds good to me 👍
There was a problem hiding this comment.
UnforwardedUnitControl sounds good to me, too! For me, the naming is less about being perfect and more about clearly communicating to the reader what's happening, and Unforwarded is nice and clear to me 😃
| <UnitControl | ||
| { ...args } | ||
| value={ value } | ||
| onChange={ ( v, extra ) => { |
There was a problem hiding this comment.
Speaking of event handlers, I was going to ask if the Actions addon would be useful or not. Especially since the work you're doing lately involves a lot of value inspection. (No strong opinion personally, I don't have experience with the addon)
| story: | ||
| "It is possible to pass a custom list of units. Every time the unit changes, if the `isResetValueOnUnitChange` is set to `true`, the input's quantity is reset to the new unit's default value.", | ||
| }, |
There was a problem hiding this comment.
Any reason this one is not in a JSDoc?
There was a problem hiding this comment.
My linter complains that I'm not adding @param tags, and so I've decided to go for the lazier approach 😅
What do you think we should do here?
There was a problem hiding this comment.
Ah, that 😅 A quick workaround is to use non-JSDoc comments, like single-asterisk /* foo or single-line // foo. I guess we could consider adding a eslintrc override on stories/* files if we see other contributors getting confused?
| */ | ||
| onChange?: UnitControlOnChangeCallback; | ||
| /** | ||
| * Size of the control option. Supports "default" and "small". | ||
| * | ||
| * @default 'default' | ||
| * @default default |
There was a problem hiding this comment.
Oh I was wondering about this "quote or no quote" problem too! It does look better this way in the props table, but I wonder if that's just a quirk in the docgen/Storybook or it's actually the correct JSDoc format. My google-fu failed me there.
There was a problem hiding this comment.
I thought about it a bit more, and I've decided to add the quotes back (it feels like the correct thing to do).
We may want to open an issue in the Storybook repo (or the ts docgen repo) to improve the way default string values are displayed?
ciampo
force-pushed
the
refactor/unit-control-storybook
branch
from
b5f6e92
to
af9553f
Compare
|
Thank you all for the review! I'll wait until tomorrow in case of any last-minute feedback, and then I'll proceed to merge (flaky e2e tests permitting) |
…pEnabled`, `shiftStep`)
ciampo
force-pushed
the
refactor/unit-control-storybook
branch
from
af9553f
to
212bb08
Compare
What?
This PR converts
UnitControl's Storybook examples to TypeScript. While doing so, it also refactors those examples from using knobs to using controls.Why?
Part of the
@wordpress/components's TypeScript migration (#35744).Altogether, the migration to TypeScript and the refactor to controls allow Storybook to extract prop types correctly and display them in the "docs" tab.
How?
UnitControl's story to TypeScriptUnitControlcomponent (necessary to extract TS props correctly)hideLabelFromVision,isShiftStepEnabled,shiftStep) (these types should be later removed onceNumberControlis migrated to TypeScript, since at that pointUnitControlwill be able to directly extendNumberControl's props)Testing Instructions
npm run distclean && npm cinpm run storybook:devUnitControlstories to the ones currently on the stable branchScreenshots or screencast
unit-control-storybook-ts.mp4