Fix blue palette table link contrast - #150
Conversation
|
Hi @abbybowman, just following up on this PR when you have a chance. The code checks are passing, but the Chromatic visual regression check is currently blocked by a missing project token in the repository configuration. I’d appreciate your review, and please let me know if any changes are needed. Thank you! |
abbybowman
left a comment
There was a problem hiding this comment.
Thanks again for this @sean-camara! A few changes requested below
There was a problem hiding this comment.
I think we can skip this one. The existing PaletteA11y story already renders AllVariants (which includes interactiveTable with real links) across all six palettes including blue, so the surface your fix targets is already exercised there and will reflect this fix (see screenshot below from Chromatic). We have a limited Chromatic snapshot budget on the free plan so trying to leverage our existing Stories whenever possible, just like you did in #149!
| --hds-palette-link-text: #{$hds-color-spacesuit-white}; | ||
| --hds-palette-link-underline: #{$hds-color-carbon-30}; | ||
| --hds-palette-link-arrow: #{$hds-color-carbon-30}; | ||
|
|
There was a problem hiding this comment.
I'd drop these three lines. On the dark and black palettes the ancestor palette already supplies the correct dark link vars, and the table's dark surface (Carbon 90) is the same as the dark palette background, so this block re-declares values that are already right.
It also drifts slightly: --hds-palette-link-arrow is set to Carbon 30 here, but our dark scheme uses Carbon 40 (_palettes.scss:106). So this actually introduces a small inconsistency (see Chromatic screenshot below).
I think the base .usa-table block is all that's needed to fix the blue bug, but if you'd recommend keeping an explicit dark block for self-documentation, that would make sense to me too; let's just match the scheme exactly (arrow → Carbon 40).
Thank you, will look out for your updated PR and get it turned around more quickly, sorry this review took a bit -- greatly appreciate you jumping in to help fix these bugs!
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@nasa/hds-core': patch | |||
There was a problem hiding this comment.
I know main changed underneath you, so just a note to update this line to match our final NPM package name: '@nasa-hds/core': patch
What this PR does
Keeps links legible when a light table is rendered inside the blue palette. Tables now own the link color variables appropriate to their fixed surface: dark text and underline colors for light tables, and the existing light treatment for dark tables.
The root cause was that links inherited white text from the surrounding blue palette even though the table itself intentionally renders on a white surface, producing 1:1 contrast.
Closes #146
Type of change
Checklist
npm run format:fixandnpm run lint:scss:fixpassnpm run lint:jsandnpm run lint:mdpassnpm test)Public API and changesets
npm run check:api-snapshotand reviewed the resultVisual review
The new
Components/Table/Links on blue paletteregression story renders the interactive table on the blue palette and verifies that link text uses the table cell's dark foreground color. Existing palette accessibility stories continue to exercise all six palettes.Notes for reviewers
Validation completed locally:
npm run buildnpm test(241 tests)npm run lintnpm run format:fixnpm run lint:scss:fixnpm run check:api-snapshotnpm run check:tokens