Button: refactor Storybook to controls and align docs #44105
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Package] Components
Mamaduka
requested review from
mirka and
ciampo
and removed request for
ajitbohra
|
Size Change: 0 B Total Size: 1.28 MB ℹ️ View Unchanged
|
ciampo
approved these changes
Sep 13, 2022
|
Thanks for the review, @ciampo. I will push the updates tomorrow.
I need to check if the state is correctly applied for all variations. But happy to add controls where it makes sense. |
Mamaduka
force-pushed
the
add/button-is-busy-knobs
branch
from
fce9c59 to
2cb890c
Compare
|
@ciampo, I converted knobs to controls, which made adding the Notes
P.S. Working with controls is way more fun 😄 |
ciampo
reviewed
Sep 27, 2022
There was a problem hiding this comment.
Thank you! The new stories are testing well !
Working with controls is way more fun 😄
I know, right?! Much easier, more fun and definitely more informative too
I combined most of the stories into two stories - "Default" and "Icon." I think this makes more sense than having multiple stories per variation.
Sounds good to me
I left "Buttons" and "Grouped Icon" stories untouched, as they are just a showcase.
While "Grouped Icon" in a way represents an example of usage of Button, I'm not sure that the "Buttons" story should stick around — it only shows a subset of all the possible combinations that the Button component can have, and all of its examples can be completely replicated via the "default" story. I would argue that it can be deleted.
I was thinking the same but wasn't 100% sure. I'm going to remove that story. |
Mamaduka
force-pushed
the
add/button-is-busy-knobs
branch
from
525715a to
dca0075
Compare
|
@ciampo, let me know if this is good to merge. |
ciampo
reviewed
Oct 11, 2022
There was a problem hiding this comment.
While giving this PR a final look, I noticed some discrepancies between the docs and the code regarding most default values of Button's props. Looking at the code, almost every prop doesn't have a default value (i.e. its values is undefined when the prop is not passed to Button) apart from iconPosition, which has a default value of left. But the README lists default values for almost every prop.
Since we shouldn't have this discrepancy, we have two choices:
- Update the code to mirror what the docs say
- Update the docs to be in line with the code
In order to keep the scope of this PR small and focused on docs, I suggest that we update both the README and the Storybook stories to remove all default values (apart from iconPosition).
In the README, when there's an implicit default value assigned by other components, we may want to mention it explicitly - e.g.:
diff --git a/packages/components/src/button/README.md b/packages/components/src/button/README.md
index 3bf23aed95..a3f2106221 100644
--- a/packages/components/src/button/README.md
+++ b/packages/components/src/button/README.md
@@ -209,11 +209,10 @@ If provided, renders an [Icon](/packages/components/src/icon/README.md) componen
#### iconSize
-If provided with `icon`, sets the icon size.
+If provided with `icon`, sets the icon size. Please refer to the [Icon](/packages/components/src/icon/README.md) component for more details regarding the default value of its `size` prop.
- Type: `Number`
- Required: No
-- Default: `20 when a Dashicon is rendered, 24 for all other icons.`
#### iconPosition
The same approach could be used for the tooltipPosition prop.
We can always come back to Button and add default values (both in code and in the docs) in a separate follow-up PR.
ciampo
changed the title
|
Sorry for the delay on this one. I've removed defaults from README/story. Also documented |
ciampo
reviewed
Oct 20, 2022
There was a problem hiding this comment.
Changes are looking good!
Apart from removing __experimentalIsFocusable's default value, the last thing is to add a CHANGELOG entry. After that, we're ready to merge!
Regarding the follow-ups mentioned around the different inline styles, I think that a good summary would be:
- Refactor the component to TypeScript, and updating the README to include all props (e.g
describedBy) - Potentially look into adding default values for those props, as it was documented in the README until this PR (and update types/readme accordingly)
Sorry for the delay on this one.
Not an issue at all! I actually really appreciate your help here :)
Co-authored-by: Marco Ciampini <[email protected]>
Co-authored-by: Marco Ciampini <[email protected]>
Mamaduka
force-pushed
the
add/button-is-busy-knobs
branch
from
8c9aa93 to
e84813a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
PR adds
isBusystate knobs to Button component stories.Testing Instructions
npm run storybook:devprimary,secondaryorisDestructive.Screenshots or screencast