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-4226: Document current button offerings ("big", "outline") #193

Merged
merged 5 commits into from
Mar 1, 2021

Conversation

aduth
Copy link
Member

@aduth aduth commented Feb 25, 2021

This pull request...

  • Updates Buttons page to include Big button examples.
  • Small adjustments to Big button sizing per Figma design (padding, font size, line height).
  • Renames "Secondary" button to "Outline" button
    • Note: In code, "Secondary" still exists, and is effectively aliased as the outline button. This is how it was implemented previously, and otherwise we'd inherit USWDS secondary button styling, which may not be desirable or at least not predictable in its usage.

Note: Between this and #191, whichever is merged first, the other should include and verify the behavior of the unstyled, big button. I tested this locally and it appeared to work reasonably well out of the box.

Screenshot:

Big Buttons

Live Preview: https://federalist-2f194a10-945e-4413-be01-46ca6dae5358.app.cloud.gov/preview/18f/identity-style-guide/aduth-lg-4226-document-outline-big/components/buttons/

<button class="usa-button usa-button--big">
```

<div>
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: The seemingly-unnecessary <div> wrapper exists to prevent the button previews from being wrapped in <p>, which in the context of the page .usa-prose causes premature line wrapping.

It might make sense to apply this to all of the examples.

@aduth
Copy link
Member Author

aduth commented Feb 26, 2021

Status: Currently working on reconciling this with unstyled button from #191. Will push an update shortly.

Edit: Now up-to-date.

@aduth aduth force-pushed the aduth-lg-4226-document-outline-big branch from 67385e7 to 8493781 Compare February 26, 2021 18:50
@aduth aduth force-pushed the aduth-lg-4226-document-outline-big branch from 8493781 to f202b18 Compare February 26, 2021 19:05
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

Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

Looking great! Couple tiny adjustments:

  • All big button styles (except Unstyled buttons of course) should have .5rem/8px border radius
  • For both regular and big Outline buttons: hover and active state text color should match the border color (hover = primary-dark, active = primary-darker)

@aduth
Copy link
Member Author

aduth commented Mar 1, 2021

Thanks @anniehirshman-gsa ! All good catches.

  • All big button styles (except Unstyled buttons of course) should have .5rem/8px border radius

This is fixed in 6f7e3e1.

  • For both regular and big Outline buttons: hover and active state text color should match the border color (hover = primary-dark, active = primary-darker)

This is updated in 69a13a0. It appears that we were explicitly overriding this before. I just removed that, so the color should match the border. On closer inspection, there seemed to be quite a bit we were overriding from USWDS that was redundant, so as part of this, I was able to simplify the styles a decent amount.

Copy link
Contributor

@anniehirshman-gsa anniehirshman-gsa left a comment

Choose a reason for hiding this comment

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

Sounds great! 👍

@aduth aduth merged commit f66d22a into main Mar 1, 2021
@aduth aduth deleted the aduth-lg-4226-document-outline-big branch March 1, 2021 20:36
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