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

Button: Include explicit state classes in unstyled hover, active overrides #4077

Merged
merged 5 commits into from
Apr 22, 2021

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Mar 10, 2021

Description

The unstyled button mixin customizes the hover and active appearance of a button, but only applies this with the browser-native :hover and :active modifiers, and not with USWDS' explicit usa-button--hover and usa-button--active modifier classes.

Additional information

Before these changes, if one would apply usa-button--hover to an unstyled button, the overrides in these styles would not be applied, most notably the background color. You can test this by adding usa-button--hover class to the "Unstyled button" example in the Buttons component page.

Before After
before after

Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

…ides

The unstyled button mixin customizes the hover and active appearance of a button, but only applies this with the browser-native `:hover` and `:active` modifiers, and not with USWDS' explicit `usa-button--hover` and `usa-button--active` modifier classes.
@aduth
Copy link
Contributor Author

aduth commented Mar 10, 2021

I just noticed this also happens with disabled unstyled buttons, regardless if disabling is applied by browser-native disabled attribute or using the .usa-button--disabled modifier class.

Screenshot:

disabled button

I could include a fix for that here as well, which I expect would amount to expanding the existing selector to also include...

&:disabled,
&.usa-button--disabled,

@aduth
Copy link
Contributor Author

aduth commented Mar 12, 2021

Another thing I'm noticing is that we rely on the typeset-link mixin to apply text color for states, but that only applies those styles using :hover, :active, and :focus.

@aduth
Copy link
Contributor Author

aduth commented Mar 12, 2021

Another thing I'm noticing is that we rely on the typeset-link mixin to apply text color for states, but that only applies those styles using :hover, :active, and :focus.

Extended for hover and active states in 9cb66ff . usa-focus works well as-is.

@aduth
Copy link
Contributor Author

aduth commented Apr 5, 2021

Today I encountered another conflict where a button can be both :active and :disabled, which also conflicts with styles between the unstyled button and default disabled button styles in _button-disabled.scss:

&:hover,
&.usa-button--hover,
&:active,
&.usa-button--active,
&:focus,
&.usa-focus {
background-color: color("disabled");
border: 0;
box-shadow: none;
}

&:disabled {
@include button-disabled;
}

The selector being revised here could be expanded to include those additional combination of states, to ensure that the background color is being reset correctly to transparent:

  &.usa-button:disabled:hover,
  &.usa-button:disabled.usa-button--hover,
  &.usa-button:disabled:active,
  &.usa-button:disabled.usa-button--active,
  &.usa-button:disabled:focus,
  &.usa-button:disabled.usa-focus

Context: 18F/identity-idp#4881

@mejiaj
Copy link
Contributor

mejiaj commented Apr 20, 2021

The only outstanding thing is this comment about disabled states in uswds/src/stylesheets/elements/_buttons.scss.

@aduth
Copy link
Contributor Author

aduth commented Apr 20, 2021

The only outstanding thing is this comment about disabled states in uswds/src/stylesheets/elements/_buttons.scss.

Oops, I'd meant to revisit this sooner, though I'm glad I'd at least left a comment about the finding 😅

I pushed a change for this in 8a4a326. Overall I wish it wouldn't be necessary to have such a large grouping of selectors, though there's a lot of different states to account for. This should now account for the disabled hover, active, and focus styles for the mixin's usage in both .usa-button:disabled and .usa-button--disabled.

@thisisdano thisisdano merged commit c3d224f into uswds:develop Apr 22, 2021
@thisisdano thisisdano mentioned this pull request Apr 27, 2021
@aduth aduth deleted the aduth-unstyled-hover-active-classes branch May 4, 2021 12:50
aduth added a commit to 18F/identity-idp that referenced this pull request Aug 15, 2022
aduth added a commit to 18F/identity-idp that referenced this pull request Aug 16, 2022
* Remove upstream-patched disabled button styling

See: uswds/uswds#4077

* Limit active disabled button styles to default button

**Why**: Because the styling was only implemented with the expectation of use with the default button, and conflicts with other button states (e.g. unstyled buttons used as the submit buttons on some forms).

Previously: #6564

changelog: Improvements, Forms, Use pressed state appearance for buttons disabled on form submission
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