-
Notifications
You must be signed in to change notification settings - Fork 922
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
USWDS - Button group: Fix margins on nested button groups #5885
Conversation
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.
Lgtm! Two non blocking comments but this works for me 👍
Testing checklist
- Confirm that elements in nested button groups are the same height as each other
- Confirm that elements in nested button groups are the same height as their parent buttons
- Confirm the code meets standards
packages/usa-button-group/src/test/test-patterns/usa-button-group--test-nested.twig
Outdated
Show resolved
Hide resolved
packages/usa-button-group/src/test/test-patterns/usa-button-group--test-nested.twig
Outdated
Show resolved
Hide resolved
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.
Overall looks good. Two comments.
I tested mobile, tablet, and desktop sizes.
height: 100%; | ||
|
||
.usa-button-group__item { | ||
display: flex; |
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.
Is this property required? Not seeing any effect on test when disabled.
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.
Good catch. It looks like this isn't needed anymore. I removed this in 8a926e3
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.
The test prefix is added at the start of the filename. For example: test-usa-button-group--nested.twig
.
I realize component template is hardcoded, but in the future we should avoid additional hardcoded components to allow for easier test template creation. As-is this hardcoded test template is okay.
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.
I updated the file names in the button group /test-patterns/
directory to reflect the common naming convention for these files in 85f2945
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.
Looks good. I tested in Chromium 125, Firefox 127, and Safari 17.4.1.
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.
Fixed indents in df884c8.
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.
Thank you!
Summary
Improved styles for nested button groups. Now, nested button groups should match the height of their parents.
Breaking change
This is not a breaking change.
Related issue
Closes #5825
Related pull requests
Changelog PR
Preview link
Nested button group test story
Problem statement
Buttons in nested button groups have mismatched heights.
Solution
This PR adds styles for nested button groups so that the nested buttons are the same height as their parents.
Testing and review