docs(modal): introduce title-only modal examples #4703
Conversation
|
Deploy preview for the-carbon-components ready! Built with commit 9e8366f https://deploy-preview-4703--the-carbon-components.netlify.com |
|
Deploy preview for carbon-components-react ready! Built with commit 9e8366f https://deploy-preview-4703--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 9e8366f |
| Passive modal title as the message. Should be direct and 3 lines or less. | ||
| `.trim() | ||
| ), | ||
| modalAriaLabel: text( |
There was a problem hiding this comment.
Does this need to be a knob?
There was a problem hiding this comment.
Yes so people can test change in the prop.
There was a problem hiding this comment.
What would they be testing by changing it?
| 'A label to be read by screen readers on the modal root node' | ||
| ), | ||
| size: select('Size (size)', sizes, 'sm'), | ||
| iconDescription: text( |
There was a problem hiding this comment.
Does this need to be a knob?
There was a problem hiding this comment.
Yes so people can test change in the prop.
|
@aagonzales Updated with the option to have buttons. Thanks! |
|
@asudoh I noticed that in the title-only example, the danger knob doesn't change anything like it does in the other examples, should it? |
| @@ -153,6 +187,25 @@ storiesOf('Modal', module) | |||
| }, | |||
| } | |||
| ) | |||
| .add( | |||
| 'Title only', | |||
There was a problem hiding this comment.
Any chance we could have this new example be without knobs? As we're adding stories, might make more sense to drop knob usage as they can make it harder to see how to use the component from the story.
There was a problem hiding this comment.
The knobs are still needed for heading content, etc. I think the discussion was around a story variant like this title only example, which was introduced as a story variant instead of a knob.
There was a problem hiding this comment.
Hmm, not sure I understand. Just was speaking to the Title only example being hard to know how to use because of the knobs. In other words, this snippet doesn't really help an external user understand how to use the component easily as it has the knob abstraction applied to it
This is something that's been coming up in our dev channels where we have been talking about not using knobs because they can make it harder to use Storybook as a reference for how to use a component.
There was a problem hiding this comment.
I see your point about the snippet readability. I'd like to keep the ability to demonstrate different options here, esp. to keep the consistency of other modal demos.
There was a problem hiding this comment.
This isn't to suggest that we have to remove how we demonstrate these options, just was speaking to how we're choosing to demonstrate these options through knobs and how that makes things hard for existing content. Would be great if new content didn't fall into the same problem 👍
There was a problem hiding this comment.
Do you want to include that to your v11 proposal list? I'd like to keep the knobs usage consistent across other modal stories for this PR, but we can always revisit the pattern in future.
There was a problem hiding this comment.
Definitely understand 👍 Since it's related to docs, doubt we need to wait for v11 but can be an effort over time to update.
|
@abbeyhrt At https://deploy-preview-4703--carbon-components-react.netlify.com/?path=/story/modal--title-only, changing |
|
Good point @emyarod - Updated. |
Refs #4607.
Changelog
New
Testing / Reviewing
Testing should make sure modal stories are not broken.