feat(checkbox/group): s2 token migration, control spacing, subtle color changes #3531

aramos-adobe · 2025-01-30T18:59:06Z

Checkbox/Group S2 Migrations

Checkbox has some new tokens for control and color. Most of the tokens have been updated in the global vars.

New tokens

--spectrum-component-size-width-ratio-down
--spectrum-checkbox-bottom-to-text(S,M,L,XL)
--spectrum-checkbox-top-to-control (S,M,L,XL)
--spectrum-accent-content-color-default
--spectrum-accent-content-color-hover
--spectrum-accent-content-color-down
--spectrum-accent-content-color-key-focus

Modified tokens

--spectrum-checkbox-checkmark-color
--spectrum-checkbox-invalid-color-down
--spectrum-checkbox-control-color-default
--spectrum-checkbox-control-color-hover
--spectrum-checkbox-control-color-down
--spectrum-checkbox-control-color-focus

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

Look at the checkbox component in storybook
Check light and dark modes in Testing Mode

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:

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

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-01-30T18:59:10Z

🦋 Changeset detected

Latest commit: 10afa8e

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/checkbox	Major

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-01-30T19:01:19Z

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

github-actions · 2025-01-30T19:03:52Z

File metrics

Summary

Total size: 1.37 MB^*

Package	Size	Minified	Gzipped
checkbox	19.44 KB	18.53 KB	2.48 KB

checkbox

Filename	Head	Minified	Gzipped	Compared to base
index.css	19.44 KB	18.53 KB	2.48 KB	🟢 ⬇ 3.25 KB
metadata.json	10.54 KB	-	-	🟢 ⬇ 2.21 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.

rise-erpelding

This is a great start! I pointed out some of the changes that I thought were still needed to the CSS within the CSS file, but here's a quick rundown of what I think needs another look:

Down state
Some of the invalid default colors (emphasized colors seem to be working fine for whatever reason) - this is a (hopefully small) bugfix that I also noticed on main
Typography use of tokens
Updating CJK to match other components
Probably removing some stray dist files
There are SO many test cases covered already, but for some reason we don't have any focus, hover, or active states. I don't think there's any particular reason we've left them out as far as I know, so I'm wondering how you would feel about adding them? I think it would make catching bugs (like the bug I just mentioned above) a little easier!

As far as I can tell, I think the template.js and stories.js files for this component are fine, so probably no changes needed there!

components/checkbox/index.css

rise-erpelding · 2025-01-31T16:20:41Z

components/checkbox/dist/index.css.map

Should we have this components/checkbox/dist/index.css.map and components/checkbox/dist/index.css committed?

rise-erpelding · 2025-01-31T16:21:18Z

components/progresscircle/dist/index.css

Wondering if these dist files for progresscircle also need to be removed.

components/checkbox/index.css

jawinn · 2025-02-20T17:22:52Z

components/checkbox/index.css

+		/* WHCM settings */
+		.spectrum-Checkbox-box,
+		.spectrum-Checkbox-input:checked + .spectrum-Checkbox-box,
+		&.is-indeterminate .spectrum-Checkbox-input:checked + .spectrum-Checkbox-box,
+		&.is-indeterminate .spectrum-Checkbox-box {
+			&::before {
+				border-color: var(--highcontrast-checkbox-color-default);
+			}
+		}
+
+		&:hover {
+			.spectrum-Checkbox-box::before,
+			.spectrum-Checkbox-input:checked + .spectrum-Checkbox-box::before {
+				border-color: var(--highcontrast-checkbox-color-default);
+			}


~~High contrast only styles should live in the forced-colors media query.~~ ✅

Also could these possibly be handled by changes to custom property values instead?

components/checkbox/index.css

components/checkbox/stories/checkbox.test.js

.changeset/six-donuts-march.md

…ut out some tests

…s2-migration

rise-erpelding

Left a comment about the testing grid!

components/checkbox/stories/checkbox.test.js

…s2-migration

rise-erpelding

Leaving a question/comment about :active, but the testing grid looks great!

Since I was already in selector land and thinking about the :active pseudo-class, I have one other question/thought that I couldn't put in the code, some of @jawinn's previous feedback was:

I noticed this when testing the pseudo states and considering our :active selectors. The down state is not shown when tabbing to a checkbox and selecting it with space, which I'm guessing it should? I think this could be resolved by moving the :active to also apply to the input and not the root element---that'd need some testing.

That seems valid and hasn't been done yet, would something like this work? (Down around L170, inside of .spectrum-Checkbox but after :hover and :after--this isn't new code in this PR so GitHub won't let me comment inline.)

	&:not(.is-readOnly) {
		.spectrum-Checkbox-input:active:not(:disabled) + .spectrum-Checkbox-box,
		&:active .spectrum-Checkbox-input:not(:disabled) + .spectrum-Checkbox-box {
			transform: perspective(var(--spectrum-component-size-minimum-perspective-down)) translateZ(var(--spectrum-component-size-difference-down));
		}
	}

components/checkbox/index.css

…s2-migration

…ss, and finding indeterminate bug

…tps://github.com/adobe/spectrum-css into aramos-adobe/css1016-checkbox-group-s2-migration

rise-erpelding

All of my feedback items have been addressed and this is looking pretty good to me! Thanks for all your work on this!!

jawinn

Really close on this one. Thanks for addressing my last set of feedback.

On the PR link I'm seeing one background color difference on the Docs page for invalid indeterminate. It's not showing the darker red on focus like the regular checked:
In the testing preview grid refactoring, it looks like we lost some states that were important to test, such as the one that would catch number 1 above:
Focused + checked, focused + indeterminate, Focused + checked + invalid, focused + indeterminate + invalid, Hover + checked, hovered + indeterminate, hover + checked + invalid, hover + indeterminate + invalid, the same for read-only states, hover on disabled states

components/checkbox/index.css

rise-erpelding · 2025-04-17T20:01:20Z

Really close on this one. Thanks for addressing my last set of feedback.

On the PR link I'm seeing one background color difference on the Docs page for invalid indeterminate. It's not showing the darker red on focus like the regular checked:

In the testing preview grid refactoring, it looks like we lost some states that were important to test, such as the one that would catch number 1 above:
Focused + checked, focused + indeterminate, Focused + checked + invalid, focused + indeterminate + invalid, Hover + checked, hovered + indeterminate, hover + checked + invalid, hover + indeterminate + invalid, the same for read-only states, hover on disabled states

😱 This is my bad. Yeah, I think the way we have the testing grid set up is a little too simple. Since some of these states (disabled, read only, indeterminate) need to cross with each other, we'd have to set up test cases with them as testData as well as being stateData, perhaps?

What if testData looked something like:

testData: [
		{
			testHeading: "Default",
		},
		{
			testHeading: "Emphasized",
			isEmphasized: true,
		},
		{
			testHeading: "Invalid with states",
			isInvalid: true,
		},
		{
			testHeading: "Checked with states",
			isChecked: true,
		},
		{
			testHeading: "Indeterminate with states",
			isIndeterminate: true,
		},
		{
			testHeading: "Disabled with states",
			isDisabled: true,
		},
		{
			testHeading: "Read-only with states",
			isReadOnly: true,
		},
		{
			testHeading: "Truncation",
			withStates: false,
			label: "Checkbox with an extraordinarily long label. Please don't do this but if you did, it should wrap text when it gets longer than the container that houses the checkbox with the unacceptably long label",
			customStyles: {
				"max-width": "200px",
			}
		}
	],

and stateData:

stateData: [
		{
			testHeading: "Checked",
			isChecked: true,
			ignore: ["Checked with states"],
		},
		{
			testHeading: "Indeterminate",
			isIndeterminate: true,
			ignore: ["Indeterminate with states"],
		},
		{
			testHeading: "Hovered",
			isHovered: true,
		},
		{
			testHeading: "Active",
			isActive: true,
		},
		{
			testHeading: "Focused",
			isFocused: true,
		},
		{
			testHeading: "Invalid",
			isInvalid: true,
			ignore: ["Invalid with states"],
		},
		{
			testHeading: "Invalid, checked",
			isInvalid: true,
			isChecked: true,
			ignore: ["Invalid with states"],
		},
		{
			testHeading: "Invalid, indeterminate",
			isInvalid: true,
			isIndeterminate: true,
			ignore: ["Invalid with states", "Indeterminate with states"],
		},
		{
			testHeading: "Disabled",
			isDisabled: true,
			ignore: ["Disabled with states"],
		},
		{
			testHeading: "Disabled, checked",
			isDisabled: true,
			isChecked: true,
			ignore: ["Disabled with states"],
		},
		{
			testHeading: "Disabled, indeterminate",
			isDisabled: true,
			isIndeterminate: true,
			ignore: ["Disabled with states", "Indeterminate with states"],
		},
		{
			testHeading: "Read-only",
			isReadOnly: true,
			ignore: ["Read-only with states"],
		},
		{
			testHeading: "Read-only, checked",
			isReadOnly: true,
			isChecked: true,
			ignore: ["Read-only with states", "Checked with states"],
		},
		{
			testHeading: "Read-only, indeterminate",
			isReadOnly: true,
			isIndeterminate: true,
			ignore: ["Read-only with states", "Indeterminate with states"],
		},
		{
			testHeading: "Read-only, checked, disabled",
			isReadOnly: true,
			isChecked: true,
			isDisabled: true,
			ignore: ["Read-only with states", "Checked with states", "Disabled with states"],
		},
	],

I still don't think that's perfect, since it leaves out Focused + checked + invalid, focused + indeterminate + invalid, and possibly others. Open to other ideas for sure but that might be a good first step if nothing else.

aramos-adobe · 2025-04-17T20:18:44Z

@rise-erpelding + @jawinn, just pushed some changes to the test grid before seeing Rise's suggestion. What do y'all think of this? I'm going to try Rise's suggestion as well to see what it looks like 😁

testData: [ { testHeading: "Default", }, { testHeading: "Emphasized", isEmphasized: true, }, { testHeading: "Truncation", withStates: false, label: "Checkbox with an extraordinarily long label. Please don't do this but if you did, it should wrap text when it gets longer than the container that houses the checkbox with the unacceptably long label", customStyles: { "max-width": "200px", } } ],

`stateData: [
	{
		testHeading: "Hovered",
		isHovered: true,
	},
	{
		testHeading: "Checked",
		isChecked: true,
	},
	{
		testHeading: "Indeterminate",
		isIndeterminate: true,
	},
	{
		testHeading: "Hovered + checked",
		isHovered: true,
		isChecked: true,
	},
	{
		testHeading: "Hovered + indeterminate",
		isHovered: true,
		isChecked: true,
	},
	{
		testHeading: "Hovered + indeterminate + invalid",
		isHovered: true,
		isChecked: true,
		isIndeterminate: true,
	},
	{
		testHeading: "Active",
		isActive: true,
	},
	{
		testHeading: "Focused",
		isFocused: true,
	},
	{
		testHeading: "Focused + checked",
		isFocused: true,
		isChecked: true,
	},
	{
		testHeading: "Focused + indeterminate",
		isFocused: true,
		isIndeterminate: true,
	},
	{
		testHeading: "Invalid",
		isInvalid: true,
	},
	{
		testHeading: "Invalid, checked",
		isInvalid: true,
		isChecked: true,
	},
	{
		testHeading: "Invalid, indeterminate",
		isInvalid: true,
		isIndeterminate: true,
	},
	{
		testHeading: "Hovered + checked + invalid",
		isHovered: true,
		isChecked: true,
		isInvalid: true,
	},
	{
		testHeading: "Focused + checked + invalid",
		isFocused: true,
		isChecked: true,
		isInvalid: true,
	},
	{
		testHeading: "Focused + indeterminate + invalid",
		isFocused: true,
		isIndeterminate: true,
		isInvalid: true,
	},
	{
		testHeading: "Read-only, checked + invalid",
		isReadOnly: true,
		isChecked: true,
		isInvalid: true,
	},
	{
		testHeading: "Read-only, indeterminate + invalid",
		isReadOnly: true,
		isIndeterminate: true,
		isInvalid: true,
	},
	{
		testHeading: "Disabled",
		isDisabled: true,
	},
	{
		testHeading: "Disabled, checked",
		isDisabled: true,
		isChecked: true,
	},
	{
		testHeading: "Disabled, indeterminate",
		isDisabled: true,
		isIndeterminate: true,
	},
	{
		testHeading: "Read-only",
		isReadOnly: true,
	},
	{
		testHeading: "Read-only, checked",
		isReadOnly: true,
		isChecked: true,
	},
	{
		testHeading: "Read-only, checked + hovered",
		isReadOnly: true,
		isChecked: true,
		isHovered: true,
	},
	{
		testHeading: "Read-only, indeterminate",
		isReadOnly: true,
		isIndeterminate: true,
	},
	{
		testHeading: "Read-only, indeterminate + hovered",
		isReadOnly: true,
		isIndeterminate: true,
		isHovered: true,
	},
	{
		testHeading: "Read-only, checked, disabled",
		isReadOnly: true,
		isChecked: true,
		isDisabled: true,
	},
],`

Screen.Recording.2025-04-17.at.4.16.45.PM.mov

aramos-adobe · 2025-04-17T20:25:36Z

Just added "hovered and invalid"

…s2-migration

aramos-adobe added ready-for-review S2 Spectrum 2 labels Jan 30, 2025

aramos-adobe self-assigned this Jan 30, 2025

aramos-adobe force-pushed the aramos-adobe/css1016-checkbox-group-s2-migration branch from 8a329ce to 6ca8078 Compare January 30, 2025 19:09

aramos-adobe requested a review from rise-erpelding January 30, 2025 19:13

rise-erpelding previously requested changes Jan 31, 2025

View reviewed changes

castastrophe force-pushed the spectrum-two branch 2 times, most recently from 56c6806 to 793571c Compare February 5, 2025 17:33

pfulton force-pushed the spectrum-two branch from ff3adf1 to f5b76b3 Compare February 6, 2025 21:29

castastrophe force-pushed the spectrum-two branch 3 times, most recently from f34473b to cf46c2b Compare February 7, 2025 20:01

aramos-adobe closed this Feb 7, 2025

aramos-adobe force-pushed the aramos-adobe/css1016-checkbox-group-s2-migration branch from 0a7d424 to cf46c2b Compare February 7, 2025 20:58

aramos-adobe reopened this Feb 7, 2025

aramos-adobe force-pushed the aramos-adobe/css1016-checkbox-group-s2-migration branch from f6bfaa1 to 988b92a Compare February 10, 2025 16:35

aramos-adobe requested a review from rise-erpelding February 11, 2025 16:10

castastrophe force-pushed the spectrum-two branch 4 times, most recently from dee81f5 to ce2f3e0 Compare February 11, 2025 19:53

jawinn self-requested a review February 11, 2025 21:12

aramos-adobe force-pushed the aramos-adobe/css1016-checkbox-group-s2-migration branch from 644902d to f77a28f Compare February 11, 2025 22:12

jawinn suggested changes Feb 12, 2025

View reviewed changes

aramos-adobe requested a review from jawinn February 13, 2025 16:52

jawinn suggested changes Feb 20, 2025

View reviewed changes

castastrophe force-pushed the spectrum-two branch from 95af748 to 5923e29 Compare February 24, 2025 20:09

aramos-adobe added 3 commits April 15, 2025 14:38

feat(checkbox): cleaned house - reorg classes, removed bad selects, c…

8347e7f

…ut out some tests

Merge branch 'spectrum-two' into aramos-adobe/css1016-checkbox-group-…

db69ea6

…s2-migration

chore(checkbox): restoring search metadata

7395bc4

aramos-adobe requested review from jawinn and rise-erpelding April 15, 2025 19:12

rise-erpelding reviewed Apr 16, 2025

View reviewed changes

components/checkbox/stories/checkbox.test.js Show resolved Hide resolved

aramos-adobe added 3 commits April 16, 2025 12:26

Merge branch 'spectrum-two' into aramos-adobe/css1016-checkbox-group-…

85b0d1e

…s2-migration

feat(checkbox): adding active to test grid

ab0c919

Merge branch 'spectrum-two' into aramos-adobe/css1016-checkbox-group-…

e7b27f1

…s2-migration

rise-erpelding reviewed Apr 16, 2025

View reviewed changes

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

aramos-adobe added 3 commits April 16, 2025 19:43

Merge branch 'spectrum-two' into aramos-adobe/css1016-checkbox-group-…

b5b8419

…s2-migration

feat(checkbox): adding checkbox downstate fix, removing is-active cla…

57cac73

…ss, and finding indeterminate bug

Merge branch 'aramos-adobe/css1016-checkbox-group-s2-migration' of ht…

19c52a6

…tps://github.com/adobe/spectrum-css into aramos-adobe/css1016-checkbox-group-s2-migration

rise-erpelding approved these changes Apr 17, 2025

View reviewed changes

jawinn suggested changes Apr 17, 2025

View reviewed changes

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

feat(checkbox): add back test cases

6ee6161

feat(checkbox): adding hovered invalid

8b3b070

feat(checkbox): disable all hover states in disabled state

a8c2404

jawinn approved these changes Apr 18, 2025

View reviewed changes

Merge branch 'spectrum-two' into aramos-adobe/css1016-checkbox-group-…

10afa8e

…s2-migration

aramos-adobe merged commit e4556f1 into spectrum-two Apr 18, 2025
12 checks passed

aramos-adobe deleted the aramos-adobe/css1016-checkbox-group-s2-migration branch April 18, 2025 21:12

This was referenced May 5, 2025

docs: adds tag for migrated components #3688

Merged

docs: add storybook migrated badge #3690

Merged

chore: add missing tags #3692

Merged

This was referenced May 16, 2025

chore(swatch,menu,swatchgroup): fix reverted metadata assets #3752

Merged

chore: merge latest updates from main #3777

Merged

feat(checkbox/group): s2 token migration, control spacing, subtle color changes #3531

feat(checkbox/group): s2 token migration, control spacing, subtle color changes #3531

Uh oh!

Conversation

aramos-adobe commented Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checkbox/Group S2 Migrations

New tokens

Modified tokens

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

Uh oh!

changeset-bot bot commented Jan 30, 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 Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

File metrics

Summary

checkbox

Uh oh!

rise-erpelding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rise-erpelding Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

rise-erpelding Jan 31, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jawinn Feb 20, 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rise-erpelding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rise-erpelding left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rise-erpelding left a comment

Choose a reason for hiding this comment

Uh oh!

jawinn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rise-erpelding commented Apr 17, 2025

Uh oh!

aramos-adobe commented Apr 17, 2025

Uh oh!

aramos-adobe commented Apr 17, 2025

Uh oh!

Uh oh!

Uh oh!

aramos-adobe commented Jan 30, 2025 •

edited

Loading

changeset-bot bot commented Jan 30, 2025 •

edited

Loading

github-actions bot commented Jan 30, 2025 •

edited

Loading

github-actions bot commented Jan 30, 2025 •

edited

Loading

jawinn Feb 20, 2025 •

edited

Loading