Skip to content
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 stories for AlignmentToolbar and AlignmentControl components #67046

Merged
merged 6 commits into from
Nov 27, 2024

Conversation

miminari
Copy link
Member

What?

This PR will add stories for AlignmentToolbar and AlignmentControl components in the Storybook

Why?

Most of Block Editor components don't have stories.
Block Editor components's Storybook should also be added and updated like the Components.
see #66519
related #22891

How?

I have referred to the TextAlignmentControl story.
AlignmentControl and AlignmentToolbar are based on same component, but there are deliberately generated as a component with a different name, so I created in two separate files.

Testing Instructions

npm run storybook:dev and please check the AlignmentControl and AlignmentToolbar stories.

Testing Instructions for Keyboard

Screenshots or screencast

Kapture.2024-11-16.at.13.37.50.mp4

@miminari miminari requested a review from ellatrix as a code owner November 16, 2024 04:42
@miminari miminari self-assigned this Nov 16, 2024
Copy link

github-actions bot commented Nov 16, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: miminari <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: mirka <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@miminari miminari added Storybook Storybook and its stories for components [Type] Developer Documentation Documentation for developers labels Nov 16, 2024
@miminari miminari requested review from mirka and t-hamano and removed request for ellatrix November 16, 2024 04:43
@miminari miminari marked this pull request as draft November 16, 2024 04:47
@miminari miminari marked this pull request as ready for review November 16, 2024 04:54
@miminari
Copy link
Member Author

@t-hamano Thank you for quick reviewing!
I updated codes. Could you please check again?

Comment on lines 33 to 47
const Template = ( { onChange, ...args } ) => {
const [ value, setValue ] = useState();
return (
<AlignmentToolbar
{ ...args }
onChange={ ( ...changeArgs ) => {
onChange( ...changeArgs );
setValue( ...changeArgs );
} }
value={ value }
/>
);
};

export const Default = Template.bind();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, but just as a tip for easier authoring and maintenance, it will be a lot simpler to showcase the component in uncontrolled mode if the component supports it. It will literally be a one-liner 😄

Suggested change
const Template = ( { onChange, ...args } ) => {
const [ value, setValue ] = useState();
return (
<AlignmentToolbar
{ ...args }
onChange={ ( ...changeArgs ) => {
onChange( ...changeArgs );
setValue( ...changeArgs );
} }
value={ value }
/>
);
};
export const Default = Template.bind();
export const Default = {};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, so easy! And more ideal. Thanks for letting me know!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then we can't reproduce the onChange moving on Storybook, but that's not a problem, right? Or I guess I might miss something...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onChange should still be observable in the Actions panel as before. Is it not working in your environment? The only reason we need to merge the incoming onChange function is when it's used in controlled mode and you need to call the setValue as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my environment, the story of the AlignmentToolbar component does not work properly. The onChange event is fired, but the text alignment is not updated. Does this component not support uncontrolled mode?

d5e018c76f3413012303cbe0189ea76b.mp4

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, my bad! I guess it doesn't support uncontrolled. I'd still recommend using CSF 3 format (instead of the Template.bind()) for new stories though:

export const Default = {
	render: function Template( { onChange, ...args } ) {
		const [ value, setValue ] = useState();
		return (
			<AlignmentToolbar
				{ ...args }
				onChange={ ( ...changeArgs ) => {
					onChange( ...changeArgs );
					setValue( ...changeArgs );
				} }
				value={ value }
			/>
		);
	},
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you two! I updated as you wrote.

@miminari miminari requested a review from mirka November 27, 2024 00:07
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@mirka mirka merged commit fedbea3 into WordPress:trunk Nov 27, 2024
64 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.9 milestone Nov 27, 2024
@miminari miminari deleted the add/alignment-control-stories branch November 28, 2024 02:31
michalczaplinski pushed a commit that referenced this pull request Dec 5, 2024
…onents (#67046)

* Add stories for AlignmentToolbar and AlignmentControl components

* Remove unneccesery args

* package.json: remove packageManager entry

* Rename the story file and remove unnecessary args from AlignmentControl story

* Update packages/block-editor/src/components/alignment-control/stories/aliginment-toolbar.story.js

Co-authored-by: Lena Morita <[email protected]>

* Add render function

---------

Co-authored-by: miminari <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: mirka <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Storybook Storybook and its stories for components [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants