-
Notifications
You must be signed in to change notification settings - Fork 1k
USWDS - Table: Update color styles to use theme settings #4712
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
Conversation
…e-color-settings
@@ -5,13 +5,12 @@ | |||
@use "../utilities" as *; | |||
@use "../typography/typeset" as *; | |||
|
|||
$table-text-color: color( |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the color function here so that we could use this variable with next-token()
below for hover states
@@ -184,7 +184,7 @@ $theme-summary-box-link-color: default !default; | |||
$theme-summary-box-text-color: default !default; | |||
|
|||
// Table | |||
$theme-table-border-color: default !default; | |||
$theme-table-border-color: "ink" !default; |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$theme-table-border-color
needs a defined token value or it will throw an error. ink
is the current rendered default value.
|
||
&[aria-sort] { | ||
color: $table-sorted-header-text-color; | ||
} | ||
} | ||
/* stylelint-disable selector-class-pattern */ | ||
th[data-sortable]:not([aria-sort]) { | ||
.usa-table__header__button .usa-icon > g.unsorted { | ||
fill: color($table-text-color); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a default fill for unsorted icons to match the header color
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we double check the variables for color setting (lines 9-73)? There are some backgrounds being set as "white" with preferred text token of "white".
$preferred-text-token: $theme-text-reverse-color, | ||
$fallback-text-token: $theme-table-text-color, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the default $theme-table-text-color
be the preferred token?
$bg-color: $theme-body-background-color // "white"
$preferred-text-token: $theme-text-reverse-color, // "white"
$fallback-text-token: $theme-table-text-color, // default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I will test out using the settings as the preferred token, not the fallback, throughout table.scss
); | ||
|
||
$table-header-text-color: color( | ||
get-color-token-from-bg( | ||
$bg-color: $theme-table-header-background-color, | ||
$preferred-text-token: $theme-text-reverse-color, | ||
$fallback-text-token: $theme-table-header-text-color, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved the theme settings variables into the value for $preferred-text-token
. Allowed $fallback-text-token
to use the fallback from project defaults.
$table-sorted-icon-color: color( | ||
get-color-token-from-bg( | ||
$bg-color: $theme-table-sorted-header-background-color, | ||
$preferred-text-token: $theme-table-sorted-icon-color, | ||
$context: "Table sorted icon", | ||
) | ||
); | ||
|
||
$table-unsorted-icon-color: get-color-token-from-bg( | ||
$bg-color: $theme-table-header-background-color, | ||
$preferred-text-token: $theme-table-unsorted-icon-color, | ||
$context: "Table unsorted icon", | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run $table-sorted-icon-color
and $table-unsorted-icon-color
through a contrast check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks Amy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work
Description
Closes #4389
Problem
Theme table settings are not consistently applied to table styles. This results in an unpredictable user experience and undesired visual presentation.
$theme-table-border-color
is defined but not used$theme-table-sorted-icon-color
is defined but not used$theme-table-sorted-icon-color
and$theme-table-unsorted-icon-color
do not check for accessible contrast ratioAn example that demonstrates most of these issues can be seen by customizing the following variables, which returns the screenshot below:
Note that the table border and sort icons are not receiving defined values, and that the unsorted icon on the borderless table does not have acceptable contrast ratio
Solution
$theme-table-border-color
and$theme-table-sorted-icon-color
to define related style rules$theme-table-sorted-icon-color
and$theme-table-unsorted-icon-color
through a contrast check to increase the likelihood of an acceptable contrast token.$preferred-text-token
rather than placing that value in$fallback-text-token
Testing
To test, simply customize
$theme-table
colors and check the rendered results. I've included some samples below as a starting point.Desired result: Rendered colors should match the declared settings, and should differ from these settings only when the supplied color does not meet contrast requirements. If no accessible colors are available, the user should receive a compile warning and instructions to choose another color.
LimitationsThere are limited instances when the functions will return defaults instead of settings values. See dark body background section below for an example.Light body background
This demo uses the same settings as above. Note that all colors are being rendered as expected, except for the borderless unsorted icon, which instead is delivering a color with accessible contrast. The header text color is changed to white because the provided color does not create accessible contrast ratio.
Dark body background
If a user sets both$theme-text-reverse-color
and$theme-table
settings to dark colors, the$theme-table
settings color will be overwritten with project defaults, even if it has an accessible contrast ratio. In this case below,$theme-table-header-text-color
and$theme-table-stripe-text-color
are being overwritten, despite having an accessible contrast ratio.Because all settings here have been given values with accessible color contrast, these colors are rendering as expected with no warnings.
Default settings
Before you hit Submit, make sure you’ve done whichever of these applies to you:
npm test
and make sure the tests for the files you have changed have passed.