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

LG-6485: Update line height value for accordion component #316

Merged
merged 2 commits into from
Jun 3, 2022

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 3, 2022

Why: To improve readability when the accordion heading spans multiple lines of text.

Live preview: https://federalist-2f194a10-945e-4413-be01-46ca6dae5358.app.cloud.gov/preview/18f/identity-style-guide/aduth-lg-6485-accordion-line-height/components/accordions/

Screenshots:

Before After
Screen Shot 2022-06-03 at 10 47 39 AM Screen Shot 2022-06-03 at 10 46 57 AM

**Why**: To improve readability when the accordion heading spans multiple lines of text.
@aduth aduth requested a review from nickttng June 3, 2022 14:51
@aduth aduth changed the title LGDS: Update line height value for accordion component LG-6485: Update line height value for accordion component Jun 3, 2022
Copy link
Member

@nickttng nickttng left a comment

Choose a reason for hiding this comment

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

This looks good. TY!

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth
Copy link
Member Author

aduth commented Jun 3, 2022

The visual regression failure highlighted an interesting side-effect of this change: Since the "here's how you know" of the USA banner is implemented as an accordion, it was impacted by these changes, slightly increasing the height of the banner. I can bring that specific accordion button back down to the smaller line-height if you'd rather it not change, @nickttng ?

@nickttng
Copy link
Member

nickttng commented Jun 3, 2022

@aduth After testing across browsers and widths, I think a slight increase is okay. Are you okay with it, too? Particularly if/when this is in effect with the IdP.

@aduth
Copy link
Member Author

aduth commented Jun 3, 2022

@aduth After testing across browsers and widths, I think a slight increase is okay. Are you okay with it, too? Particularly if/when this is in effect with the IdP.

I think it'd be fine too.

Only other thing worth pointing out is I discovered USWDS does define some explicit line-height for the banner text ("Official website...") equivalent to 1.2, and previously the accordion button's line-height didn't matter so much since it was smaller and the text + button were vertically centered. We could opt to style the banner accordion button to 1.2 to bring them back in sync?

@nickttng
Copy link
Member

nickttng commented Jun 3, 2022

We could opt to style the banner accordion button to 1.2 to bring them back in sync?

Given the USWDS discovery, this makes sense. 👍🏼

Decrease from 1.5 to 1.2 to match

See: #316 (comment)
@aduth
Copy link
Member Author

aduth commented Jun 3, 2022

Given the USWDS discovery, this makes sense. 👍🏼

Cool, I think so too 👍 Updated in 812f87a.

@nickttng
Copy link
Member

nickttng commented Jun 3, 2022

Just inspected the latest built. Looking good.

@aduth
Copy link
Member Author

aduth commented Jun 3, 2022

Current visual regression build failure looks more expected now, limited to just the accordions themselves:

image

@aduth aduth merged commit 4296481 into main Jun 3, 2022
@aduth aduth deleted the aduth-lg-6485-accordion-line-height branch June 3, 2022 16:42
@aduth aduth mentioned this pull request Jun 6, 2022
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.

None yet

3 participants