docs(badge): add playground example - #8110
Conversation
9286357 to
f906471
Compare
nuria1110
left a comment
nuria1110
left a comment
There was a problem hiding this comment.
Looking good, just a few comments from me.
Not sure if we would prefer excluding props from the controls instead of picking out which ones to show? We might miss including some props that could be useful or forget to add any new future props into the list.
We will also need to make sure we are disabling chromatic on these stories since most of the files have this enabled.
| counter: { | ||
| control: { | ||
| type: "text", | ||
| type: "number", |
There was a problem hiding this comment.
comment (non-blocking): The prop accepts both string and number so I would personally leave this as text.
| size: { | ||
| options: ["small", "medium", "large"], | ||
| control: { type: "radio" }, | ||
| }, | ||
| variant: { | ||
| options: ["typical", "subtle"], | ||
| control: { type: "radio" }, | ||
| }, |
There was a problem hiding this comment.
comment: Unless we need to customise the options or prop types I don't think we need to redefine the control types as it infers them from the prop types, so I don't think this is necessary for most of the components.
| inverse: false, | ||
| }, | ||
| }; | ||
| Playground.storyName = "Playground"; |
There was a problem hiding this comment.
comment (non-blocking): If we are using CSF3 stories I don't think its necessary to provide a storyName for this as it just uses the story export.
| export const Playground: Story = { | ||
| render: (args) => { | ||
| const [isChecked, setIsChecked] = useState(false); | ||
| return ( | ||
| <Checkbox | ||
| {...args} | ||
| checked={isChecked} | ||
| onChange={(e) => setIsChecked(e.target.checked)} | ||
| /> | ||
| ); | ||
| }, | ||
| args: { | ||
| label: "Checkbox", | ||
| disabled: false, | ||
| reverse: false, | ||
| required: false, | ||
| size: "medium", | ||
| }, | ||
| }; | ||
| Playground.storyName = "Playground"; |
There was a problem hiding this comment.
suggestion: We could just reuse the ControlledCheckbox here:
| export const Playground: Story = { | |
| render: (args) => { | |
| const [isChecked, setIsChecked] = useState(false); | |
| return ( | |
| <Checkbox | |
| {...args} | |
| checked={isChecked} | |
| onChange={(e) => setIsChecked(e.target.checked)} | |
| /> | |
| ); | |
| }, | |
| args: { | |
| label: "Checkbox", | |
| disabled: false, | |
| reverse: false, | |
| required: false, | |
| size: "medium", | |
| }, | |
| }; | |
| Playground.storyName = "Playground"; | |
| export const Playground: Story = { | |
| render: ControlledCheckbox, | |
| args: { | |
| label: "Checkbox", | |
| disabled: false, | |
| reverse: false, | |
| required: false, | |
| size: "medium", | |
| }, | |
| }; |
| <Canvas of={DecimalStories.WithPopoverContainer} /> If both props are supplied, | ||
| only the prefix will be rendered and a console warning will be logged. Use one | ||
| or the other. |
There was a problem hiding this comment.
question: Aware this was there before these changes but this sentence seems a bit out of place, not sure if its meant to go with the Suffix story instead?
|
|
||
| <Controls | ||
| of={ProgressTrackerStories.Playground} | ||
| include={["progress", "description", "labelsPosition"]} |
There was a problem hiding this comment.
comment: I feel like we're missing some props here that could also be useful like currentProgressLabel, maxProgressLabel, etc
|
|
||
| <Controls | ||
| of={RadioButtonGroupStories.Playground} | ||
| include={["legend", "legendSpacing"]} |
There was a problem hiding this comment.
comment: Same here, we should probably include a few more props, legendSpacing is also deprecated so we shouldn't include it.
| export const Playground: Story = { | ||
| render: (args) => { | ||
| const [value, setValue] = useState(""); | ||
| return ( | ||
| <RadioButtonGroup | ||
| {...args} | ||
| name="playground-group" | ||
| value={value} | ||
| onChange={(ev) => setValue(ev.target.value)} | ||
| > | ||
| <RadioButton id="playground-1" value="radio1" label="Radio Option 1" /> | ||
| <RadioButton id="playground-2" value="radio2" label="Radio Option 2" /> | ||
| <RadioButton id="playground-3" value="radio3" label="Radio Option 3" /> | ||
| </RadioButtonGroup> | ||
| ); | ||
| }, | ||
| args: { | ||
| legend: "RadioButtonGroup Legend", | ||
| legendSpacing: 2, | ||
| }, | ||
| }; | ||
| Playground.storyName = "Playground"; |
There was a problem hiding this comment.
suggestion: We could also use ControlledRadioButtonGroup here instead.
f906471 to
427c73b
Compare
333816d to
3a8eeed
Compare
|
Good work for trying to get on top of this. Initial observation is that as part of introducing Playgrounds we should try and reduce the complexity of our docs pages at the same time. I also noticed that some of the Playgrounds don't cover all the available props. I'll try and comment on the individual stories where possible, but some might be missed. |
nineteen88
left a comment
nineteen88
left a comment
There was a problem hiding this comment.
I've messaged on Slack rather than adding too many comments, I think there's some general changes we can make overall as alluded to in my first comment on the PR. Really good so far. Trying to improve the way we present our component documentation to users is always appreciated!
|
|
||
| <Canvas of={TypographyStories.Playground} /> | ||
|
|
||
| <Controls of={TypographyStories.Playground} include={["children", "variant"]} /> |
There was a problem hiding this comment.
suggestion: include a lot more of the available controls
The customisation a user can do is very limited if we only allow children and variant. We should open it up to allow much more. Maybe the following?
[ "children", "variant", "size", "color", "weight", "fluid", "inverse", "screenReaderOnly", ]
|
|
||
| <Controls of={TypographyStories.Playground} include={["children", "variant"]} /> | ||
|
|
||
| ## Designer Notes |
There was a problem hiding this comment.
suggestion: move the Designer Notes above the Playground example
|
|
||
| ## Examples | ||
|
|
||
| ### Variants |
There was a problem hiding this comment.
suggestion: keep the variants story but remove the others
It would probably be okay to only have the variants story as a way to showcase all the different variants. The other stories are probably redundant at that point, unless you feel we really need to explain certain behaviour. The hope would be that the prop names combined with verbose descriptions of them should cover that for the majority of cases though.
There was a problem hiding this comment.
note: whenever removing stories, we do need to ensure that we maintain Chromatic coverage though. So may require moving into test stories instead.
|
|
||
| <Controls | ||
| of={TimeStories.Playground} | ||
| include={["label", "disabled", "readOnly", "required", "size"]} |
There was a problem hiding this comment.
question: could we include a control that allows you to enable the AM/PM toggle?
It should be possible, and it would be a nice way to illustrate how it's done without needing an additional story. Users should be able to see the code that builds it in the show code tab of the Playground, and if we have a descriptive control then the story would be redundant.
| of={TimeStories.Playground} | ||
| include={["label", "disabled", "readOnly", "required", "size"]} | ||
| /> | ||
|
|
There was a problem hiding this comment.
suggestion: remove unnecessary stories that are covered by the playground
3 participants
Proposed behaviour
Adds
Playgroundexamples for all components that have currently been aligned with current Fusion DS.Current behaviour
Only
Pillhas aPlaygroundexample.Checklist
d.tsfile added or updated if requiredQA
Additional context
N/A
Testing instructions
New examples should have controls that work.
Please note that
ButtonTogglehas an args tab and inside this tab there are no controls present. I cannot disable this tab, if I do the args forButtonToggleGroupalso disappear.