-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Storybook : Add TextTransformControl stories #67365
Storybook : Add TextTransformControl stories #67365
Conversation
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @im3dabasia! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I would like to make two suggestions.
- It looks like you are adding as many variations as possible as individual stories, but this seems redundant. All these props can be changed from the Controls panel. Also, if we go ahead with this approach, we will probably add similar variations to all other block editor components 😅
- It is certainly easier to understand with demo text, but Storybook is for testing and developing a single component itself. How this component interacts with the outside world is the responsibility of the developer who uses this component, and I think it is better not to include it in the story.
With that in mind, I think it would be best to focus on just improving argTypes
in this PR. What do you think?
Hey @t-hamano, thanks for your feedback. I have tried incorporating all the feedback you shared in this PR and the issue. Please take a look at my updated work and provide feedback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I have a few small suggestions, but overall it looks good 👍
packages/block-editor/src/components/text-transform-control/stories/index.story.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/text-transform-control/stories/index.story.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/text-transform-control/stories/index.story.js
Show resolved
Hide resolved
packages/block-editor/src/components/text-transform-control/stories/index.story.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/text-transform-control/stories/index.story.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/text-transform-control/stories/index.story.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/text-transform-control/stories/index.story.js
Outdated
Show resolved
Hide resolved
6e052fb
to
236dc2c
Compare
Hey @t-hamano , I have made the changes based on your suggestions. I am attaching a screencast for the same. Please provide feedback on my work. Screen.Recording.2024-12-19.at.1.32.16.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! Sorry, there was a mistake in my previous suggestion🙇♂️
packages/block-editor/src/components/text-transform-control/stories/index.story.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/text-transform-control/stories/index.story.js
Outdated
Show resolved
Hide resolved
Hey @t-hamano, could you please have a look at this PR? I have incorporated all the feedback you suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
One last thing, could you fix a mistake in the README? The default value for the value
prop is undefined
, not "none"
, so we'll need to remove this line:
- **Default:** `none` |
Done @t-hamano , I have removed this line. Thank you very much for the review. |
Part of #67165
What?
This PR will add stories for TextTransformControl component in the Storybook.
Testing Instructions
Screen shot