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

BorderControl: Fix button styles #56730

Merged
merged 4 commits into from
Dec 8, 2023
Merged

BorderControl: Fix button styles #56730

merged 4 commits into from
Dec 8, 2023

Conversation

brookewp
Copy link
Contributor

@brookewp brookewp commented Dec 2, 2023

What?

The size of the button in BorderControl did not match the input field so there is a slight gap between their borders. This also affects BorderBoxControl.

Why?

It looks nicer. :)

How?

Removing the hardcoded height and width, adding an aspect-ratio of 1, and using size="small" instead of the deprecated isSmall for button.

Testing Instructions

Ensure BorderControl and BorderBoxControl's borders and size look correct.

Screenshots or screencast

BorderControl

Before After
Screenshot 2023-12-01 at 7 36 24 PM Screenshot 2023-12-01 at 7 36 44 PM

BorderBoxControl

Before After
Screenshot 2023-12-01 at 7 36 30 PM Screenshot 2023-12-01 at 7 36 53 PM

@brookewp brookewp added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Dec 2, 2023
@brookewp brookewp requested a review from a team December 2, 2023 03:43
@brookewp brookewp self-assigned this Dec 2, 2023
ciampo
ciampo reviewed Dec 4, 2023
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The approach is clean and works well, but currently BorderControl doesn't update correctly when size="__unstable-large"

Screenshot 2023-12-04 at 16 28 29

@brookewp
Copy link
Contributor Author

brookewp commented Dec 5, 2023

The approach is clean and works well, but currently BorderControl doesn't update correctly when size="__unstable-large"

Great catch! I was thinking of adding __next40pxDefaultSize in the other PR (#56185) but that will just be on the inputs, and here it is needed on the button.

Updated this in 602a3f4

@brookewp brookewp requested a review from a team December 5, 2023 23:35
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2023 •
edited
Loading

Flaky tests detected in 2d45a2a.
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/7135425110
📝 Reported issues:

@brookewp brookewp mentioned this pull request Dec 6, 2023
Merged
chad1008
chad1008 approved these changes Dec 7, 2023
View reviewed changes
Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

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

Looking good to me, and testing well! 🚀 🚢

@brookewp brookewp merged commit 3ad117e into trunk Dec 8, 2023
52 checks passed
@brookewp brookewp deleted the fix/bordercontrol-button branch December 8, 2023 01:29
@github-actions github-actions bot added this to the Gutenberg 17.3 milestone Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ciampo ciampo ciampo left review comments

@chad1008 chad1008 chad1008 approved these changes

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

Assignees

@brookewp brookewp

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

Successfully merging this pull request may close these issues.
3 participants
@brookewp @ciampo @chad1008