[wb-1797] Address cell color contrast a11y issues #2435
Conversation
|
Size Change: +179 B (+0.18%) Total Size: 97.8 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-dirmdfiuvc.chromatic.com/ Chromatic results:
|
🦋 Changeset detectedLatest commit: 018a3b9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…Cell: update styling to address color contrast accessibility issues
beaesguerra
force-pushed
the
wb-1797-cell-color-contrast
branch
from
080eab9 to
962fe64
Compare
| @@ -94,7 +94,7 @@ const DetailCell = function (props: DetailCellProps): React.ReactElement { | |||
|
|
|||
| const styles = StyleSheet.create({ | |||
| subtitle: { | |||
| color: color.offBlack64, | |||
| color: "#6D6F74", // TODO: use token | |||
There was a problem hiding this comment.
I'm getting feedback from design on if we can use a token for the new subtitle color (or if this is a one-off): https://www.figma.com/design/qSKXmIvmsNPKFlQNP5cztQ?node-id=19669-5006#1098834936
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (de57eb1) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2435"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2435 |
There was a problem hiding this comment.
One thing I noticed about DetailCell is that it is used by OptionItem. The updated styles aren't propagated to the OptionItems in the dropdown component because OptionItem overrides a lot of the styles. Is it expected that DetailCell and OptionItem have different styling?
There was a problem hiding this comment.
hmm good question... I would say that it's better to have different styling as OptionItems tend to be closer to "action" styles (like the button ones). My recommendation for now would be to keep it as it is for now. We could ask Design in case they want to revisit the styling for dropdowns if we want to be more consistent of course :).
… shades for text on light blue backgrounds
beaesguerra
force-pushed
the
wb-1797-cell-color-contrast
branch
from
ae53bdc to
6d0b1d5
Compare
|
Converting this PR to a draft while I get some feedback on introducing a new token for the cell subtitle text (a darked |
…OffBlack shades for text on light blue backgrounds" This reverts commit 6d0b1d5.
… semantic text secondary token to new darker gray so it works with fadedBlue8 backgrounds
…etail cell subtitle text
… color primitive token and sets the text secondary token to this primitive. The slightly darker gray has better color contrast on a variety of backgrounds, including the fadedBlue8 background
…h new token structure
There was a problem hiding this comment.
This is similar to the detail-cell-variants stories, but for CompactCell! I approved the initial Chromatic diff for adding these stories so that the diff will only show the changes in the new styles. Let me know if you have any questions!
There was a problem hiding this comment.
sounds good! thanks for the heads up.
There was a problem hiding this comment.
This looks really solid! The only "blocking" think is that I noticed an issue with this story: https://5e1bf4b385e3fb0020b7073c-ekfutacxua.chromatic.com/?path=/story/packages-dropdown-multiselect--custom-styles-opened, but maybe it's something that needs to be addressed in the story example?
.changeset/wise-actors-peel.md
Outdated
There was a problem hiding this comment.
praise: This is the best changelog I've seen in this repo 👏
There was a problem hiding this comment.
sounds good! thanks for the heads up.
| @@ -4,7 +4,7 @@ import {MemoryRouter, Route, Switch} from "react-router-dom"; | |||
| import type {Meta, StoryObj} from "@storybook/react"; | |||
|
|
|||
| import {View} from "@khanacademy/wonder-blocks-core"; | |||
| import {color, spacing} from "@khanacademy/wonder-blocks-tokens"; | |||
| import {border, color, spacing} from "@khanacademy/wonder-blocks-tokens"; | |||
There was a problem hiding this comment.
super-nit: Would it be possible to replace color with semanticColor here? no worries is you can't, but I've been doing that with my current PRs as the end goal is to use examples to advocate for using these colors instead of primitive ones.
There was a problem hiding this comment.
I will do this in a follow up PR and release the changes together!
There was a problem hiding this comment.
hmm good question... I would say that it's better to have different styling as OptionItems tend to be closer to "action" styles (like the button ones). My recommendation for now would be to keep it as it is for now. We could ask Design in case they want to revisit the styling for dropdowns if we want to be more consistent of course :).
|
|
||
| // hover + enabled | ||
| [":hover[aria-disabled=false]" as any]: { | ||
| background: color.offBlack8, | ||
| background: color.fadedBlue8, |
There was a problem hiding this comment.
suggestion: You could probably use semanticColor.status.notice.background here (and in all the similar cases in this file).
There was a problem hiding this comment.
I'll switch to using semantic tokens in a follow up PR and include this! Thanks for the suggestion!
…or so that we don't set overflow: hidden on clickable items since that was causing layout issues in OptionItems for dropdown components
| // state because setting the styles on the clickable element | ||
| // directly causes issues since overflow must be hidden for cases where | ||
| // the border is rounded | ||
| [":active[aria-disabled=false]:not([aria-current=true]) > *:first-child" as any]: |
There was a problem hiding this comment.
This selector 😅 Let me know if there's a better way to do this!
More context:
- This addresses the issue caught in Chromatic where providing a maxHeight to the MultiSelect caused the OptionItems to overlap. This was introduced when adding
overflow: hiddento the root of the cell - We need the
overflow: hiddenbecause there are custom overrides in webapp that sets the border-radius on DetailCell and we want to make sure the new left bar indicator stays within the cell - To fix this, the
overflow: hiddenand the left bar indicator:beforestyling is moved to theinnerWrapperof the cell, instead of the root - Since the pressed styling (
:active) is on the root element, we use> *:first-childto access to innerWrapper
Alternatives I considered:
- keep track of pressed state using JS and apply pressed styles directly to the innerWrapper (won't be able to capture :active state in visual regression tests)
- using an alternative way to apply the left bar indicator where we don't have to set overflow: hidden
- the left bar curves with the border radius in other alternatives I explored: https://jsfiddle.net/1dghqx0t/24/
Summary:
Updating state styles based on the new designs to address color contrast issues: https://www.figma.com/design/qSKXmIvmsNPKFlQNP5cztQ?node-id=4337-8746#1091493830
Styles to highlight:
A new primitive color token is also added:
fadedOffBlack72. This is the new value forsemanticColor.text.secondary. This is a darker gray that has better color contrast with afadedBlue8backgroundIssue: WB-1797
Test plan:
?path=/docs/packages-cell-detailcell-all-variants--docs?path=/docs/packages-cell-compactcell-all-variants--docs