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

Allow non-token values in theme color settings #3258

Merged
merged 4 commits into from Dec 12, 2019
Merged

Conversation

thisisdano
Copy link
Member

@thisisdano thisisdano commented Dec 11, 2019

Allow non-token values in theme color settings. While USWDS promotes and encourages using our system tokens in theme color settings, agencies have a real need to occasionally use non-token colors. This includes instances where certain colors are mandated by law and cannot be easily changed. Now, teams can add non-token colors to theme color settings like $theme-color-primary: #f00. This new non-token value of 'primary' will apply anywhere the 'primary' token is used: functions, mixins, settings, and utilities. Using non-token values will throw a warning in the compile process, but this, like all compile warnings, can be disabled by setting $theme-show-compile-warnings: false.

Fixes #3209

Success criteria

  • Any valid color can be added a theme palette color variable
  • Non-token color overrides compile with a useful warning
  • Compile warning may be disabled with $theme-show-compile-warnings: false
  • Non-color, non-token assignments result in a useful error
  • Assigned color compiles and outputs properly
  • Assigned color works in mixins
  • Assigned color works in functions
  • Assigned color works in utilities
  • Assigned color works as a setting (ex: $theme-link-color)
  • New code is organized, efficient, and readable
  • Passes all CI tests

@thisisdano thisisdano added Type: Enhancement An improvement to existing code improves: continuity labels Dec 11, 2019
@thisisdano thisisdano added this to the Release 2.4.0 milestone Dec 11, 2019
@thisisdano thisisdano requested a review from a team December 11, 2019 00:39
@thisisdano thisisdano self-assigned this Dec 11, 2019
@@ -123,7 +123,7 @@ Leaves bools as is

@if type-of($value) == "color" {
@error 'Only use quoted color tokens in USWDS functions and mixins. '
+ 'See v2.designsystem.digital.gov/style-tokens/color '
+ 'See designsystem.digital.gov/design-tokens/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.

Fixed this url as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably update this on:

src/stylesheets/settings/_settings-color.scss:16
src/stylesheets/theme/_uswds-theme-color.scss:16

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in every instance I could find

Comment on lines +906 to +920
// Non-token colors may be passed with specific flags
@if type-of($value) == color {
// override or set-theme will allow any color
@if index($flags, override) or index($flags, set-theme) {
// override + no-warn will skip warnings
@if index($flags, no-warn) {
@return $value;
}

@if $theme-show-compile-warnings {
@warn 'Override: `#{$value}` is not a USWDS color token.';
}

@return $value;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The color function can now include a number of flags like color(value, flag-1, flag-2, ...). override and set-theme flags allow passing a color. no-warn suppresses the warning.

// False values may be passed through when setting theme colors
@if $value == false {
@if index($flags, set-theme) {
@return $value;
Copy link
Member Author

Choose a reason for hiding this comment

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

Rewrote this to restrict the false pass-through to set-theme only — this allows the color() function to be used when setting the theme token (since theme tokens can be false).

Comment on lines +943 to +953
// If we're using the theme flag, $project-color-shortcodes has not yet been set
@if not index($flags, set-theme) {
@if map-has-key($project-color-shortcodes, $value) {
$our-color: (map-get($project-color-shortcodes, $value));
@if $our-color == false {
@error '`#{$value}` is a color that does not exist '
+ 'or is set to false.';
}
@return $our-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.

Improves error handling. We can't check against the theme colors when we're setting them, so setting the set-theme flag bypasses this check

Comment on lines +955 to +957
@error '`#{$value}` is not a valid USWDS color token. '
+ 'See designsystem.digital.gov/design-tokens/color '
+ 'for more information.';
Copy link
Member Author

Choose a reason for hiding this comment

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

Improves error message

@thisisdano thisisdano added this to Review in USWDS Sprint Board Dec 11, 2019
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.

LGTM, though we should update the old `style-tokens/color' url on other sections as well.

@@ -123,7 +123,7 @@ Leaves bools as is

@if type-of($value) == "color" {
@error 'Only use quoted color tokens in USWDS functions and mixins. '
+ 'See v2.designsystem.digital.gov/style-tokens/color '
+ 'See designsystem.digital.gov/design-tokens/color '
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably update this on:

src/stylesheets/settings/_settings-color.scss:16
src/stylesheets/theme/_uswds-theme-color.scss:16

@thisisdano
Copy link
Member Author

I'll update asap

@mejiaj mejiaj merged commit 57d2e78 into develop Dec 12, 2019
USWDS Sprint Board automation moved this from Review to Done Dec 12, 2019
@mejiaj mejiaj deleted the dw-non-token-theme branch December 12, 2019 21:25
@rhukster
Copy link

Greatly looking forward to this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement An improvement to existing code
Projects
Development

Successfully merging this pull request may close these issues.

Allow non-token values in theme color token assignments
3 participants