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: Improve support for unstyled button #191

Merged
merged 3 commits into from
Feb 26, 2021

Conversation

aduth
Copy link
Member

@aduth aduth commented Feb 25, 2021

The unstyled button variant had previously existed in that it was inherited from USWDS, though it was not documented, and did not always display correctly, particularly in hover, active, and focus states. These changes improve this support, and adds documentation for the unstyled option.

Screenshot:

unstyled button

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

Comment on lines +42 to +50
&:hover,
&.usa-button--hover {
color: color($theme-link-hover-color);
}

&:active,
&.usa-button--active {
color: color($theme-link-active-color);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

@anniehirshman-gsa I noticed in the Figma that the hover and active states should be different colors. I think what we have here is the correct implementation, as far as using $theme-link-hover-color and $theme-link-active-color, but it appears they're currently configured to the same color.

I think it might make sense to update that globally so that the link hover and active colors are distinct, if that's the intention. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I would like to update $theme-link-hover-color: 'primary-dark'; and $theme-link-active-color: 'primary-darker'; globally, to match the unstyled button mockups in Figma. Currently links have a hover color of primary-darker and no active color. @nickttng does that sound good to you too?

Copy link
Member

Choose a reason for hiding this comment

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

@anniehirshman-gsa Yep, that sounds good

@aduth aduth force-pushed the aduth-lg-4226-document-variants branch from ef669a3 to d17b5ad Compare February 25, 2021 18:54
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

Comment on lines +56 to +57
&.usa-button:not([disabled]):focus,
&.usa-button:not([disabled]).usa-focus {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, this is different from the stanza as below because the & means these styles when they apply to .usa-button--unstyled right, and the one below is when they're on any button?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify, this is different from the stanza as below because the & means these styles when they apply to .usa-button--unstyled right, and the one below is when they're on any button?

Yeah, when compiled it's the difference of:

.usa-button--unstyled.usa-button:not([disabled]):focus {
  /* ... these styles */
}

vs.

.usa-button:not([disabled]).usa-focus {
  /* ... default focus styles */
}

Where the intention with these styles is to unset and replace the default focus styles, only for the unstyled buttons. Ideally we could just use the default styles, but it was surprisingly difficult to reconcile the focus style box shadow + border radius, hence recreating it with a positioned pseudo-element.

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.

LGTM (with change to hover and active colors in #196 )

The unstyled button variant had previously existed in that it was inherited from USWDS, though it was not documented, and did not always display correctly, particularly in hover, active, and focus states. These changes improve this support, and adds documentation for the unstyled option.
- Use utility classes
- Improve mobile display
@aduth aduth force-pushed the aduth-lg-4226-document-variants branch from d17b5ad to 6e51ebf Compare February 26, 2021 18:29
@aduth
Copy link
Member Author

aduth commented Feb 26, 2021

(with change to hover and active colors in #196 )

It might take a few minutes to finish building, but I rebased this branch after merging #196, so the preview should reflect the new hover colors in a few minutes:

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

@aduth aduth merged commit 713a3da into main Feb 26, 2021
@aduth aduth deleted the aduth-lg-4226-document-variants branch February 26, 2021 18:40
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.

4 participants