fix(swatch): adjust disabled icon color across themes #3547

rise-erpelding · 2025-02-10T16:57:00Z

Description

The primary aim here initially was to address a regression in S1/Express styles in which swatch's disabled icon in dark themes had changed in appearance (see screenshots). However, after consulting design, we discovered that the look of the icon was incorrect in all themes (and colors).

After troubleshooting and trying to match the icon to the design spec, it appears that the issue was in the svg markup used for the disabled icon in the CSS Storybook. The CSS storybook uses the cancel icon from the icons package, while the old deprecated docs site and SWC use custom markup.

The CSS changes bring the disabled swatch icon color back to the S1 and S2 specs for all themes, using --spectrum-white regardless of theme and light/dark, and using black rgb along with the opacity token for the border color.

Additionally, template changes were made to adjust the svg in Storybook - it now matches what's used in SWC and what was previously used on the docs site.

CSS-1119

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Open the PR preview for the disabled swatch component:
- Confirm that the disabled swatch matches S2 Spec and S1 spec. This has been approved by design, but it never hurts to check from a code standpoint. [@5t3ph, @cdransf]
- Disabled swatch should not change color between light and dark modes. [@5t3ph, @cdransf]
- WHCM changes are acceptable. (I would love some feedback on this! The outline is back in WHCM, it looks similar to what it was in feat(swatch)!: migrate swatch to core tokens #1501 which is where this comment was added, that leads me to believe that this is correct, but I'm still questioning it.) [@5t3ph, @cdransf]
- There should be no other visual regressions. [@5t3ph, @cdransf]
Check template changes, changeset, metadata, and css changes
- All code changes are logical and well-documented. [@5t3ph, @cdransf]

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch: (they have not been run yet but I will run them upon code approval)

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

Previous S1 disabled Swatch for light and dark themes:

Current (on main) S1 disabled Swatch for light and dark themes, demonstrating regression for dark theme from previous:

Current (on main) S2 Foundations disabled Swatch for light and dark themes, note that colors are very similar to S1 overall:

S2 Spec (S1 spec is very similar to this):

Fix (current PR) S2 Foundations disabled Swatch for light and dark themes, S1 and Express follow the same color pattern:

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
If my change impacts other components, I have tested to make sure they don't break.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨

changeset-bot · 2025-02-10T16:57:03Z

🦋 Changeset detected

Latest commit: 93993fe

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@spectrum-css/swatch	Patch

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

github-actions · 2025-02-10T17:03:38Z

File metrics

Summary

Total size: 2.25 MB^*

Package	Size	Minified	Gzipped
swatch	11.03 KB	10.39 KB	1.85 KB

swatch

Filename	Head	Minified	Gzipped	Compared to base
index-base.css	10.76 KB	10.14 KB	1.81 KB	🔴 ⬆ 0.13 KB
index-theme.css	0.88 KB	0.87 KB	0.47 KB	🟢 ⬇ 0.08 KB
index.css	11.03 KB	10.39 KB	1.85 KB	🔴 ⬆ 0.06 KB
metadata.json	5.71 KB	-	-	🔴 ⬆ 0.06 KB
themes/express.css	0.91 KB	0.90 KB	0.51 KB	🟢 ⬇ 0.06 KB
themes/spectrum-two.css	0.89 KB	0.88 KB	0.50 KB	🟢 ⬇ 0.06 KB
themes/spectrum.css	0.91 KB	0.89 KB	0.51 KB	🟢 ⬇ 0.06 KB

* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

github-actions · 2025-02-10T17:04:29Z

🚀 Deployed on https://pr-3547--spectrum-css.netlify.app

components/swatch/index.css

rise-erpelding · 2025-02-19T16:22:17Z

components/swatch/index.css

@@ -61,7 +61,8 @@

 .spectrum-Swatch {
 	--spectrum-swatch-focus-indicator-border-radius: 8px;
-	--spectrum-swatch-icon-border-color: rgb(0 0 0 / 51%);
+	--spectrum-swatch-icon-border-color: rgba(var(--spectrum-black-rgb), var(--spectrum-swatch-disabled-icon-border-opacity));
+	--spectrum-swatch-disabled-icon-color: var(--spectrum-white);


Taking this --spectrum-swatch-disabled-icon-color custom prop out of the themes and moving it back here, S1 spec and S2 spec use the same tokens so there should not be theme differences here.

5t3ph

Updates as well as WHCM appearance checks out IMO 👍

cdransf

Ran through the validation steps and everything looks good to me.

With respect to

WHCM changes are acceptable. (I would love some feedback on this! The outline is back in WHCM, it looks similar to what it was in #1501 which is where this comment was added, that leads me to believe that this is correct, but I'm still questioning it.)

I checked this in Assistive Labs and I agree with the comment you linked to — the icon color itself provides sufficient contrast and I think a border would be unnecessary. ✨

* fix(textfield): update border and background color * fix(swatch): adjust disabled icon color across themes (#3547) * fix(textfield): update border color on keyfocus in s2 --------- Co-authored-by: [ Cassondra ] <castastrophe@users.noreply.github.com> Co-authored-by: rise-erpelding <54716846+rise-erpelding@users.noreply.github.com>

rise-erpelding added skip_vrt Add to a PR to skip running VRT (but still pass the action) S2 Spectrum 2 labels Feb 10, 2025

rise-erpelding added s1 and removed S2 Spectrum 2 labels Feb 13, 2025

castastrophe removed the s1 label Feb 13, 2025

rise-erpelding force-pushed the rise-erpelding/swatch-disabled-icon-color branch 3 times, most recently from ce05221 to c99d309 Compare February 18, 2025 16:20

rise-erpelding commented Feb 18, 2025

View reviewed changes

components/swatch/index.css Outdated Show resolved Hide resolved

rise-erpelding added ask design Questions or topics for the design team bug Results from a bug in the CSS implementation labels Feb 18, 2025

rise-erpelding changed the title ~~fix(swatch): revert s1/express disabled icon color~~ fix(swatch): adjust disabled icon color across themes Feb 18, 2025

rise-erpelding force-pushed the rise-erpelding/swatch-disabled-icon-color branch 2 times, most recently from 53a358a to 2eaa453 Compare February 19, 2025 16:20

rise-erpelding commented Feb 19, 2025

View reviewed changes

rise-erpelding marked this pull request as ready for review February 19, 2025 18:02

rise-erpelding added ready-for-review S2 Spectrum 2 storybook and removed ask design Questions or topics for the design team labels Feb 19, 2025

5t3ph approved these changes Feb 19, 2025

View reviewed changes

cdransf approved these changes Feb 20, 2025

View reviewed changes

cdransf assigned rise-erpelding Feb 20, 2025

This was referenced Feb 25, 2025

fix(textfield): update border and background color #3575

Merged

fix(picker): updated background-color and border-color #3572

Closed

rise-erpelding force-pushed the rise-erpelding/swatch-disabled-icon-color branch 2 times, most recently from ba17140 to 5181c74 Compare February 25, 2025 19:34

fix(swatch): adjust disabled icon color across themes

93993fe

rise-erpelding force-pushed the rise-erpelding/swatch-disabled-icon-color branch from 5181c74 to 93993fe Compare February 25, 2025 19:37

rise-erpelding added run_vrt For use on PRs looking to kick off VRT and removed skip_vrt Add to a PR to skip running VRT (but still pass the action) labels Feb 25, 2025

rise-erpelding merged commit 45f110c into main Feb 25, 2025
24 checks passed

rise-erpelding deleted the rise-erpelding/swatch-disabled-icon-color branch February 25, 2025 20:14

github-actions bot mentioned this pull request Feb 25, 2025

chore: release #3580

Merged

TarunAdobe pushed a commit that referenced this pull request Feb 26, 2025

fix(swatch): adjust disabled icon color across themes (#3547)

f05f078

github-actions bot mentioned this pull request Feb 26, 2025

chore: release (next) #3582

Merged

castastrophe pushed a commit that referenced this pull request Feb 27, 2025

fix(swatch): adjust disabled icon color across themes (#3547)

a751402

pfulton pushed a commit that referenced this pull request Feb 27, 2025

fix(swatch): adjust disabled icon color across themes (#3547)

b3eb5b2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(swatch): adjust disabled icon color across themes #3547

fix(swatch): adjust disabled icon color across themes #3547

Uh oh!

rise-erpelding commented Feb 10, 2025 •

edited by cdransf

Loading

Uh oh!

changeset-bot bot commented Feb 10, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Feb 10, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Feb 10, 2025 •

edited

Loading

Uh oh!

Uh oh!

rise-erpelding Feb 19, 2025 •

edited

Loading

Uh oh!

5t3ph left a comment

Uh oh!

cdransf left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

fix(swatch): adjust disabled icon color across themes #3547

fix(swatch): adjust disabled icon color across themes #3547

Uh oh!

Conversation

rise-erpelding commented Feb 10, 2025 • edited by cdransf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

Uh oh!

changeset-bot bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File metrics

Summary

swatch

Uh oh!

github-actions bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rise-erpelding Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

5t3ph left a comment

Choose a reason for hiding this comment

Uh oh!

cdransf left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rise-erpelding commented Feb 10, 2025 •

edited by cdransf

Loading

changeset-bot bot commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

github-actions bot commented Feb 10, 2025 •

edited

Loading

rise-erpelding Feb 19, 2025 •

edited

Loading

cdransf left a comment •

edited

Loading