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.

Sign up for GitHub

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

Jump to bottom

Notice: Add appropriate size props to Buttons #66593

Merged
merged 5 commits into from
Oct 30, 2024
Merged

Notice: Add appropriate size props to Buttons #66593

merged 5 commits into from
Oct 30, 2024

Conversation

mirka
Copy link
Member

@mirka mirka commented Oct 30, 2024

In preparation for #65751

What?

Adds appropriate size props to the remaining Buttons in the Notice component.

Testing Instructions

See the Notice stories in Storybook.

@mirka mirka added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Oct 30, 2024
@mirka mirka self-assigned this Oct 30, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 30, 2024 •
edited
Loading

Flaky tests detected in 6495241.
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/11598880485
📝 Reported issues:

mirka
mirka commented Oct 30, 2024
View reviewed changes
packages/components/src/notice/index.tsx
@@ -160,6 +161,7 @@ function Notice( {
</div>
{ isDismissible && (
<Button
size="small"
Copy link
Member Author

Choose a reason for hiding this comment

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

The Notice will naturally reduce to a shorter height, due to the smaller button. This matches the current Figma specs, so I'm going to assume this is correct (cc @WordPress/gutenberg-design).

Before After
Notice close button, before Notice close button, after

Copy link
Member

Choose a reason for hiding this comment

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

I'm good with this, but I'd love to get a design confirmation before we 🚢 it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems fine until we holistically revisit the Notice design.

packages/components/src/notice/index.tsx
@@ -142,6 +142,7 @@ function Notice( {

return (
<Button
__next40pxDefaultSize
Copy link
Member Author

Choose a reason for hiding this comment

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

Before After
Notice action buttons, before Notice action buttons, after

Copy link
Member

Choose a reason for hiding this comment

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

LGTM in the template validation notice:

Screenshot 2024-10-30 at 14 49 41

packages/components/src/notice/stories/index.story.tsx
@@ -109,7 +109,11 @@ export const NoticeListSubcomponent: StoryFn< typeof NoticeList > = () => {
return (
<>
<NoticeList notices={ notices } onRemove={ removeNotice } />
<Button variant="primary" onClick={ resetNotices }>
<Button
__next40pxDefaultSize
Copy link
Member Author

Choose a reason for hiding this comment

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

Reset button in story

Copy link
Member

Choose a reason for hiding this comment

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

Pre-existing, but isn't it weird that those notices have no margins by 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.

Not sure 🤷 This component is on the "needs design updates" docket though.

@mirka mirka marked this pull request as ready for review October 30, 2024 12:11
@mirka mirka requested a review from ajitbohra as a code owner October 30, 2024 12:11
@mirka mirka requested a review from a team October 30, 2024 12:11
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 30, 2024 •
edited
Loading

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: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: jameskoster <[email protected]>

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

tyxla
tyxla approved these changes Oct 30, 2024
View reviewed changes
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM from my perspective, tests well with notices I've tested across the editor 👍 🚀

packages/components/src/notice/stories/index.story.tsx
@@ -109,7 +109,11 @@ export const NoticeListSubcomponent: StoryFn< typeof NoticeList > = () => {
return (
<>
<NoticeList notices={ notices } onRemove={ removeNotice } />
<Button variant="primary" onClick={ resetNotices }>
<Button
__next40pxDefaultSize
Copy link
Member

Choose a reason for hiding this comment

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

Pre-existing, but isn't it weird that those notices have no margins by default?

packages/components/src/notice/index.tsx
@@ -160,6 +161,7 @@ function Notice( {
</div>
{ isDismissible && (
<Button
size="small"
Copy link
Member

Choose a reason for hiding this comment

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

I'm good with this, but I'd love to get a design confirmation before we 🚢 it.

packages/components/src/notice/index.tsx
@@ -142,6 +142,7 @@ function Notice( {

return (
<Button
__next40pxDefaultSize
Copy link
Member

Choose a reason for hiding this comment

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

LGTM in the template validation notice:

Screenshot 2024-10-30 at 14 49 41

@mirka mirka enabled auto-merge (squash) October 30, 2024 17:52
@mirka mirka merged commit d2b3c1f into trunk Oct 30, 2024
64 checks passed
@mirka mirka deleted the notice-button-40px branch October 30, 2024 18:27
@github-actions github-actions bot added this to the Gutenberg 19.7 milestone Oct 30, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
@mirka @tyxla
@jameskoster @karthick-murugan
Notice: Add appropriate size props to Buttons (WordPress#66593)
1de2bb8 
* Notice: Add appropriate size props to Buttons

* Add size prop to reset Button in story

* Add changelog

* Update snapshot

Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: jameskoster <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameskoster jameskoster jameskoster left review comments

@tyxla tyxla tyxla approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

@mirka mirka

Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Milestone
Gutenberg 19.7
Development

Successfully merging this pull request may close these issues.
3 participants
@mirka @jameskoster @tyxla