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

ItemGroup: Improve stories to default to bordered and separated #66191

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Oct 17, 2024

What?

Improves the ItemGroup stories to have bordered and separated styles.

Why?

Because this is the most common usage and it demonstrates better how the component is composed.

How?

Enabling isBordered and isSeparated for all stories, and inverting the "WithBorder" story to be a "WithoutBorder" one.

Testing Instructions

Spin up a Storybook instance and test ItemGroup - go through all stories and ensure bordered and separated is now the default, and the non-bordered story works correctly.

Testing Instructions for Keyboard

Same

Screenshots or screencast

Before After
Screenshot 2024-10-16 at 12 48 49 Screenshot 2024-10-16 at 12 48 35

@tyxla tyxla added [Type] Developer Documentation Documentation for developers [Package] Components /packages/components labels Oct 17, 2024
@tyxla tyxla requested a review from a team October 17, 2024 08:29
@tyxla tyxla self-assigned this Oct 17, 2024
@tyxla tyxla requested a review from ajitbohra as a code owner October 17, 2024 08:29
Copy link

github-actions bot commented Oct 17, 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: tyxla <[email protected]>
Co-authored-by: ciampo <[email protected]>

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

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

I wonder if we should also change the default snippet in the JSDoc and even add a little sentence in that snippet about usually needing to set the two properties to true for the most typical visual styles.

Comment on lines 72 to 73
isBordered: true,
isSeparated: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: for consistency with how we usually write the stories:

Suggested change
isBordered: true,
isSeparated: true,
...Default.args,

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good, done in 5e26b6f

Comment on lines 90 to 91
isBordered: true,
isSeparated: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed as part of 5e26b6f

@tyxla
Copy link
Member Author

tyxla commented Oct 17, 2024

I wonder if we should also change the default snippet in the JSDoc and even add a little sentence in that snippet about usually needing to set the two properties to true for the most typical visual styles.

This is exactly why I wanted to introduce this as a breaking change. If we need to explain this behavior, then it just doesn't make sense and needs to be changed.

If y'all agree this is the best course of action, I'm happy to follow-up and address all ItemGroup usage to invert those props.

Copy link

Flaky tests detected in 5e26b6f.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11382763697
📝 Reported issues:

@tyxla tyxla enabled auto-merge (squash) October 17, 2024 10:45
@tyxla tyxla merged commit 0a6cbb6 into trunk Oct 17, 2024
63 of 65 checks passed
@tyxla tyxla deleted the update/improve-item-group-stories branch October 17, 2024 11:01
@github-actions github-actions bot added this to the Gutenberg 19.6 milestone Oct 17, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
…Press#66191)

* ItemGroup: Improve stories to default to bordered

* Inherit default story args

Co-authored-by: tyxla <[email protected]>
Co-authored-by: ciampo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants