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

USWDS - Button group: Fix margins on nested button groups #5885

Merged
merged 11 commits into from
May 22, 2024

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Apr 24, 2024

Summary

Improved styles for nested button groups. Now, nested button groups should match the height of their parents.

  • Added styles for nested button groups so that the child elements are the same height as their parents.
  • Added a test story to show nested button groups.
  • Updated file names in button tests to match common convention.

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

  1. Confirm that elements in nested button groups are the same height as each other
  2. Confirm that elements in nested button groups are the same height as their parent buttons
  3. Confirm the code meets standards

@amyleadem amyleadem changed the title Add nested button group test story USWDS - Button group: Fix margins on nested button group Apr 24, 2024
@amyleadem amyleadem changed the title USWDS - Button group: Fix margins on nested button group USWDS - Button group: Fix margins on nested button groups Apr 24, 2024
@amyleadem amyleadem marked this pull request as ready for review April 24, 2024 22:48
Copy link
Contributor

@mahoneycm mahoneycm left a 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

Copy link
Contributor

@mejiaj mejiaj left a 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;
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

@mejiaj mejiaj left a 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed indents in df884c8.

@mejiaj mejiaj requested a review from thisisdano May 20, 2024 13:51
Copy link
Member

@thisisdano thisisdano left a comment

Choose a reason for hiding this comment

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

Thank you!

@thisisdano thisisdano merged commit 8fd9278 into develop May 22, 2024
5 checks passed
@thisisdano thisisdano deleted the al-button-group-nested branch May 22, 2024 22:00
@thisisdano thisisdano mentioned this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

USWDS - Bug: Buttons in nested button groups have mismatched heights
4 participants