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 - Accordion: Add color settings #5269
Conversation
- Add context variable - Use set-text-and-bg mixin to set accessible text on user backgrounds - Pass new background setting to accordion button & content
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.
Variables are working like a charm!
I happened to notice one color wasn't setting the appropriate color icon, as well as a high contrast discrepancy we might want to address.
- Confirmed no visual regression for accordion
- Tested setting custom accordion theme colors
- Text color updates and passes color contrast
- Icon color updates and passes color contrast
- Found failure with
yellow-20v
.
- Found failure with
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 like this as a function! Good thinking here
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 like the added functionality here! I found a couple of issues and added notes below. Let me know if you have questions!
Tests performed:
- Confirmed that the content background color is configurable with
$theme-accordion-background-color
- Confirmed that the button background color is configurable with
$theme-accordion-button-background-color
- Tested background colors of each grade (0-100) and confirmed that it correctly presents text and icon colors with acceptable color contrast
- The icon does not always return the correct color. See this comment for details.
- This is probably a separate issue, but I am not able to set background colors of grade 100: both “black” and “gray-100” throw errors
- Confirmed that hover states on
usa-accordion__button
work as expected - Confirmed that there are no visual regressions with base styles
packages/uswds-core/src/styles/functions/color/is-input-darkmode.scss
Outdated
Show resolved
Hide resolved
@@ -13,14 +21,16 @@ $accordion-border: units($theme-accordion-border-width) solid | |||
width: 100%; | |||
} | |||
|
|||
// scss-lint:disable PropertyCount |
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 scss-lint:disable PropertyCount
because I wasn't seeing the error anyways.
true, | ||
false | ||
); | ||
$input-darkmode: is-color-dark($input-bg-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.
The new function loads via @use "../../functions" as *;
import on line 4.
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.
Looking good so far. I like set-icon-from-bg
.
I am not fully done with my review (planning on coming back tomorrow to finish up), but wanted to share these items in the meantime:
- A small question about
is-color-dark
- Found a contrast issue in the checkbox component.
Let me know if you have questions!
packages/uswds-core/src/styles/functions/color/is-color-dark.scss
Outdated
Show resolved
Hide resolved
packages/uswds-core/src/styles/mixins/helpers/checkbox-and-radio-colors.scss
Outdated
Show resolved
Hide resolved
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.
This is looking great on my end! I was able to confirm the checkbox bug @amyleadem discovered but I did not find any other errors.
- Flagged colors with existing issues now showing the proper icon color
-
mint-cool-30v
which was also having the same failure is now showing the proper color icon - Unable to reproduce error with other colors
- Tests pass automated testing
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.
Ok, everything else is looking good except for the items flagged in yesterday's review! I had one additional question about an error message or fallback for set-icon-from-bg
, but I don't think it needs to block anything.
/// @param {type} $icon-light - Light icon variant. | ||
/// @output - A light or dark icon depending on the $background-color. | ||
@mixin set-icon-from-bg($background-color, $icon-dark, $icon-light) { | ||
$is-text-dark: is-color-dark($background-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.
Just checking - is this a more appropriate name?
$is-text-dark: is-color-dark($background-color); | |
$is-background-dark: is-color-dark($background-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.
Good point, changing would add clarity. Updated in e48dacc.
Also, is it safe to remove the following items from the testing section of the PR description?
|
@amyleadem I've added strikethrough to preserve the record, thanks for that note. |
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.
Approved!
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.
Happy to have this addition to the design system! 💯
- Default accordion state is free of visual regression
- Custom color variables correctly update accordion button, border, and content backbground colors
- Text and icons are updated to offer appropriate color contrast despite the background colors including hover states
-
npm run test:sass
passes
@mahoneycm @amyleadem I've removed setting icons on hover because it was causing regressions in USA Banner & Header, which rely on I've also created a new test to help avoid these types of issues in the future. |
Confirming that I see the regression on hover for both banner and header. @mejiaj let me know if there are others I missed. Some examples of the regression: Note: the icon is white in these examples because I was testing with a color of grade 40. With default colors the icon will appear black. With this update, the accordion button icon and text will have mismatched colors when it transitions from a "light" background to a "dark" one. As an example, this is what setting Does it make sense to maintain the hover checks in accordion and then override the accordion background image on hover for the banner and header(ex: remove background image on |
Dismissing my previous review because changes have been made
@amyleadem thanks for confirming. I've added your suggestions. |
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 to me! Performed the following tests:
- Confirmed that accordion button text and icon colors match on hover when
$theme-accordion-button-background-color
is set to a color of grade 40 - Confirmed that there are no regressions in banner or header
Summary
Customizable accordion colors. You can now customize the accordion button and background color settings.
Breaking change
This is not a breaking change.
Related issue
Closes #5249.
Related pull requests
Need to:
Documentation updates PR
uswds/uswds-site#2081
Preview link
Preview link:
Problem statement
Accordion background colors weren't configurable. Users had to add their own CSS overrides to customize the look and feel of accordions.
Solution
Two new accordion color settings:
$theme-accordion-background-color
default
$theme-accordion-button-background-color
base-lightest
Major changes
You can now change background colors in accordion. Accessible icons and text color are set on both default & hover state.
Full list of changes
$theme-accordion-background-color
&$theme-accordion-button-background-color
.set-text-and-bg
mixin.is-color-dark()
function to determine if color is light or dark. Used for determining icon color set. Taken fromcheckbox-and-radio-colors.scss
.set-icon-from-bg
checks the grade of the background color passed and sets a white/black icon.usa-icons-bg
.Testing and review
uswds/uswds-sandbox
attest-uswds-5269
- preview link$theme-accordion-button-background-color
.npm run test:sass
must pass [e7d695b].Visit_usa-checkbox.scss
and uncomment lines 108-113.There should be no regressions in checkbox test story.Additional tests
Click to expand to see results for previously problematic colors: yellow-20v, cyan-40, green-40. These colors were mentioned in thread below.
yellow-20v
#face00
Settings
cyan-40
#449dac
Settings
green-40
#7d9b4e
Settings