Components: migrate ConfirmDialog component's Stories from knobs to controls
#40164
Conversation
|
Size Change: 0 B Total Size: 1.23 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice, thanks for working on this!
The main thing I think we should address is that we can't have uncontrolled modals popping up in Docs view. Unless we suspect that most usages will be uncontrolled, the straightforward way to address this would be to make them all controlled examples by default, like on this doc page for the Reakit modal. Another tool we have is the inline stories option which isolates each story in an iframe.
Sidenote (cc @ciampo): I played around with type extraction on this one, and (perhaps understandably) the docgen has a problem with these conditional props. I guess there are a couple of ways around it, but I wonder if this is another reason why we should avoid complicating props conditionals as a matter of API design, like you've been noting recently. (Not saying that this one is particularly complicated, or that API design should be dictated by docgen constraints, but just a thought! Interesting how TypeScript can easily enforce complicated conditions, but how quickly it gets messy when trying to explain the rules in a human-readable format.)
I do think most uses will be Uncontrolled, but looking at the two options, I still think making the examples controlled by default makes the most sense. Getting the positioning of the iframes right feels like more trouble than it's worth (part of why Storybook recommends against that method). Also, due to how the docs pull in the code snippets, the examples remain pretty clean... the extra handler functions we'd add to the template don't even show up, so it works great! I also removed the initial "Hasn't confirmed yet" text. I think it looks cleaner to start with no text and have One final change: because we've now made the default template Controlled, we can actually use that for the |
ciampo
added
[Type] Code Quality
|
Thank you @chad1008 for working on this!
Agree with @mirka on this one
If I understand correctly, since currently all examples are using the controlled version of the component, I don't think we need a "Controlled" Story anymore? At most, we'd need a way to flag that the component can also be used in uncontrolled mode — correct?
You raise a good point — I've never been a fan of conditional APIs, and I don't really like the solution that we adopted on (Coincidentally, the same pattern is being discussed in the Finally, I have a couple of questions / observations:
|
You make a good point. I've reworked it a bit so that the default example now includes the full Controlled snippet, with the state management callbacks shown. I've then replaced the separate Controlled story with a simple Uncontrolled story instead, to demo a basic application without needing to declare
Absolutely, I definitely plan on addressing this in a follow-up!
I had the same thought, but found that the JSDoc comments I added there didn't get added to the the the auto-generated docs. I initially wondered if this was because the stories weren't in TypeScript yet, but I'm no wondering if it's a side effect of those conditional props? Are they possibly preventing Storybook from parsing them the way the would on static props? |
There was a problem hiding this comment.
I had the same thought, but found that the JSDoc comments I added there didn't get added to the the the auto-generated docs. I initially wondered if this was because the stories weren't in TypeScript yet, but I'm no wondering if it's a side effect of those conditional props? Are they possibly preventing Storybook from parsing them the way the would on static props?
I believe that the main reason why types are not coming through is because the stories are not written in TypeScript yet — apologies if I didn't explain that more clearly in my previous message.
Absolutely, I definitely plan on addressing this in a follow-up!
That would be great, thank you!
| const uncontrolledSnippet = `<> | ||
| <ConfirmDialog | ||
| onConfirm={ handleConfirm } | ||
| > | ||
| Would you like to privately publish the post now? | ||
| </ConfirmDialog> | ||
|
|
||
| <Heading level={ 1 } /> | ||
| </> | ||
| `; | ||
|
|
||
| export const Uncontrolled = Template.bind( {} ); | ||
| Uncontrolled.args = {}; | ||
| Uncontrolled.parameters = { | ||
| docs: { | ||
| description: { | ||
| story: | ||
| 'To render in Uncontrolled Mode, omit passing a boolean to the `isOpen` prop. This will allow the component to close itself without the need for an explicit callback. In Uncontrolled mode, `onCancel` is optional.', | ||
| }, | ||
| source: { | ||
| code: uncontrolledSnippet, | ||
| language: 'jsx', | ||
| type: 'auto', | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Looking at this, I'm not actually sure if we should have an uncontrolled example:
- this uncontrolled example wouldn't really add any value to the would be probably misleading (it uses the "controlled" version of the component to showcase the uncontrolled?)
- the snippet, in its current status, is somewhat incomplete (
handleConfirmis not defined anywhere, theHeadingcomponent doesn't have text and is not relevant to the example)
I still think that we should somehow document the uncontrolled mode, though. Currently, we do mention it in the README, but we should consider how we can do that effectively in Storybook if we eventually want to move all of our docs.
@mirka what do you think?
There was a problem hiding this comment.
Given that the conditional props will prevent us from getting a clean props table anyway, it might be worth considering splitting it into separate components (e.g. ConfirmDialog and ConfirmDialog.Uncontrolled?), or maybe even removing the uncontrolled version.
There was a problem hiding this comment.
That's an interesting point.
For the sake of this PR, I guess we could remove the uncontrolled example completely from this PR, and tackle the conditional props and the uncontrolled component in a separate PR
There was a problem hiding this comment.
I've just looked over the four current implementations of ConfirmDialog in Gutenberg, and currently they're all Controlled, which lends more weight to the idea of doing away with Uncontrolled altogether.
I agree with removing the Uncontrolled story for now at the very least, and we can perhaps update the component to only have the single Controlled mode in a followup - which would also eliminate the conditional props at the same time which would be a double-win.
Yes, those conditional props are the main blocker at the moment, which I don't mind punting to a separate PR. There's also another prerequisite for the type extraction to work, which is to add a named export like so. But in any case the docgen part won't work unless we do something about the conditional props. |
Let's do that in a separate PR, so that in the meantime we can wrap up the refactor to controls. Next follow-ups will be:
WDTY ? |
|
I've just pushed a couple more updates cleaning up the args in the template as @ciampo suggested and removing the Uncontrolled story for the time being. Knowing that all existing uses of the component are currently Controlled, I'm leaning more and more in favor of removing Uncontrolled as an option completely in a followup PR. Once we do that, the TypeScript conversion of these stories (another followup) should be the main thing needed to wrap this up completely! |
There was a problem hiding this comment.
Knowing that all existing uses of the component are currently Controlled, I'm leaning more and more in favor of removing Uncontrolled as an option completely in a followup PR.
Once we do that, the TypeScript conversion of these stories (another followup) should be the main thing needed to wrap this up completely!
Sounds good! Thanks for all your work on this 🚀
…ith Custom Button Labels' from knobs to controls
…ory from knobs to controls
This reverts commit 04961e7.
…prevents modals popping up on DocsPage
…e updated Controlled-by-default template
chad1008
force-pushed
the
enhance/confirmdialog-knobs-to-controls
branch
from
4fad564
to
168674a
Compare
What?
Migrate the Stories for the
ConfirmDialogcomponent from knobs to the more modern controls API. The one exception is theControlledstory, which benefits more from the current format's code snippet, so doesn't have any active Controls.Why?
See #35665 for more details, but due to knobs being deprecated, all components will eventually need to be migrated.
Testing Instructions
npm run storybook:dev