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

USWDS: Fix disabled theme color settings #5402

Merged
merged 12 commits into from
Aug 18, 2023

Conversation

amyleadem
Copy link
Contributor

@amyleadem amyleadem commented Jul 26, 2023

Summary

  • Fixed configuration errors with disabled color settings. All disabled color settings should now be configurable and accept hex color values.
  • Updated disabled settings names to match other state colors. Updated default disabled color settings values and added $theme-color-disabled-lighter and $theme-color-disabled-darker.
  • Added "disabled-lighter" and "disabled-darker" color tokens. Also updated the default values for all disabled tokens.

Breaking change

⚠️ This is a breaking change. The names and values of disabled settings and tokens have changed and will need to be updated according to the table below if they are used in your project. All disabled states must meet minimum color contrast requirements for text (4.5:1).

Related issue

Closes #5334

Related pull requests

Changelog PR

Preview link

Disabled form elements page

Problem statement

  1. $theme-color-disabled-text, $theme-color-disabled-text-reverse, and $theme-color-text-on-disabled settings return a module loop error for any custom configuration.
$theme-color-disabled-light: "magenta-10", //compiles
$theme-color-disabled: "magenta-10", //compiles
$theme-color-disabled-dark: "magenta-10", //compiles
$theme-color-disabled-text: "magenta-10", //module loop error
$theme-color-disabled-text-reverse: "magenta-10", //module loop error
$theme-color-text-on-disabled: "magenta-10", //module loop error
  1. Unlike other state color settings like $theme-color-success and $theme-color-error, the disabled color settings do not accept hex values.
$theme-color-disabled-light: #fffff, //compiles (but not currently used in the system)
$theme-color-disabled: #333333, //error: not a token
$theme-color-disabled-dark: #333333, //compiles (but not currently used in the system)
$theme-color-disabled-text: #333333, //error: not a token
$theme-color-disabled-text-reverse: #333333, //error: not a token
$theme-color-text-on-disabled: #333333, //error: not a token

Solution

  1. Added the !default flag to these settings to make them configurable in projects.
  2. Created new disabled color shortcodes and used them to replace the $theme-color-disabled- references in the Sass. Using shortcodes enables the system to accept hex code values for these settings.
  3. Renamed the disabled color settings to match the convention in other state colors ("lighter", "light", "dark", etc). This adds flexibility to the use of these settings.

Settings updates

Previous setting name Previous default value New setting name New default value New token name
-- -- $theme-color-disabled-lighter "gray-20" "disabled-lighter"
$theme-color-disabled-light "gray-10" $theme-color-disabled-light "gray-40" "disabled-light"
$theme-color-disabled "gray-20" $theme-color-disabled "gray-50" "disabled"
$theme-color-disabled-dark "gray-30" $theme-color-disabled-dark "gray-70" "disabled-dark"
-- -- $theme-color-disabled-darker "gray-90" "disabled-darker"
$theme-color-disabled-text "gray-50" [Renamed to $theme-color-disabled]
$theme-color-disabled-text-reverse "gray-40" [Renamed to $theme-color-disabled-light]
$theme-color-text-on-disabled "gray-70" [Renamed to $theme-color-disabled-dark]

Warning
The settings do not create a warning when color combinations do not meet contrast standards. Recommend we open a new issue to address this.
Update: I've opened issue #5427 to address this.

Testing and review

  • Confirm that all disabled settings are configurable
  • Confirm that all disabled settings accept hex code values
  • Confirm that the updated settings names make sense and match convention
  • Confirm that all $theme-color-disabled- references in Sass have been updated to shortcodes
  • Confirm that all new tokens work in the color function
  • Confirm no visual regressions

To test configuration:

  1. In a test project, install this branch
  2. In your project, configure USWDS by adding hex values to the following settings. Example:
@use "uswds-core" with (
    $theme-color-disabled-lighter: #999999, 
    $theme-color-disabled-light: #888888, 
    $theme-color-disabled: #777777,
    $theme-color-disabled-dark: #555555,
    $theme-color-disabled-darker: #444444,
);
  1. Confirm that Sass compiles without errors.

@amyleadem amyleadem marked this pull request as ready for review July 27, 2023 21:43
- Caused a Sass error because didn't follow color-grade format
@amyleadem
Copy link
Contributor Author

Putting this in draft while I update the disabled color settings names.

@amyleadem amyleadem marked this pull request as draft August 4, 2023 18:11
- Replace names to better align w/the naming convention in state colors
@amyleadem amyleadem marked this pull request as ready for review August 4, 2023 19:31
@amyleadem
Copy link
Contributor Author

@mejiaj @mahoneycm re-opened this PR with new settings variable names. I still need to update the changelog/documentation PR, but the code should be ready for review 👍

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.

@amyleadem nice fix. I was able to customize disabled color settings using gray-warm family.

Is there an issue created for this warning in PR description?

image

@amyleadem
Copy link
Contributor Author

@mejiaj I have opened #5427 to address the contrast warning concern.

@mejiaj
Copy link
Contributor

mejiaj commented Aug 7, 2023

Great, thank you!

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.

Looking great!

  • Confirm that all disabled settings are configurable
  • Confirm that all disabled settings accept hex code values
  • Confirm that the updated settings names make sense and match convention
  • Confirm that all $theme-color-disabled- references in Sass have been updated to shortcodes
  • Confirm that all new tokens work in the color function
  • Confirm no visual regressions

@thisisdano
Copy link
Member

Trying to wrap my head around the breaking implications of this change.

If I look at this situation from the original issue:

@use 'uswds-core' with (
  $theme-color-disabled: #767676
);

After this work, this will no longer result in a compile error, but $theme-color-disabled no longer applies in the same way, internally.

Is it correct to say that if a project previously customized $theme-color-disabled they now should customize $theme-color-disabled-lighter since we now use that value for internal styling in the places we previously used $theme-color-disabled?

And if folks referenced $theme-color-disabled in custom Sass, they should update those references to $theme-color-disabled-lighter?

@amyleadem
Copy link
Contributor Author

@thisisdano Yes, that is a good clarification.

@thisisdano thisisdano merged commit 7bd8130 into develop Aug 18, 2023
3 checks passed
@thisisdano thisisdano deleted the al-fix-disabled-theme-colors branch August 18, 2023 16:46
@thisisdano thisisdano mentioned this pull request Aug 18, 2023
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: Disabled color settings configuration errors
4 participants