Skip to content

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
@amyleadem amyleadem requested review from mejiaj and mahoneycm 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
Contributor

@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!

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