feat(table): migrates remaining table styles to s2 part2 #3818

marissahuysentruyt · 2025-05-21T20:48:14Z

Description

This PR is part 2 of the table's migration to S2! Table has been updated to include:

new color & typography tokens (in the table Figma token specs, only tokens for Colors & Typography have been addressed)
new sparkline cell content type and respective story

NOTE: Tokens for layout and storybook template updates were done in part 1 of the table migration here: #3799

All styles for the full migration should now be addressed with this PR.

Jira/Specs

CSS-1179
Figma token specs- colors and typography token columns only

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

Please review the following table styles to ensure the color and typography tokens are used as outlined in the tokens spec Figma file:

Additional validation

All test cases are covered
I'd love feedback on the approach I took to achieving only 1px between adjacent selected rows, since I'm afraid there may be content shift. That is Stefan's preference (to only have 1px of border between adjacent selected cells). But he understands that's a complex thing to achieve without content shift, so he said that the 2px in between would be acceptable if we really needed it.

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-05-21T20:48:18Z

🦋 Changeset detected

Latest commit: dbfb1cf

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

This PR includes changesets to release 4 packages

Name	Type
@spectrum-css/tokens	Patch
@spectrum-css/table	Minor
@spectrum-css/preview	Patch
@spectrum-css/bundle	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

marissahuysentruyt · 2025-05-29T20:55:00Z

TODO: (from Stephanie in #3799)

For WHCM - I think there should be a difference between "Focused selected" vs. "Focused unselected" just like there is in regular mode:
#3799 (comment)

EDIT: The focused unselected ONLY has the side focus indicator, while a focused + selected has the side focus indicator AND the background color change. ✅

Possibly need a background color for the table header for "Quiet" styling to prevent this clash on scrollable tables:
#3799 (comment)

EDIT: This was a transparent background color before, but with the updates to the header cell background colors, this should be taken care of. ✅

tokens/custom/dark-vars.css

marissahuysentruyt · 2025-06-03T14:48:39Z

components/table/index.css

-	&.spectrum-Table--quiet {
-		border-block-start: none;
-	}


Quiet tables still have a border-block-start for the header row.

marissahuysentruyt · 2025-06-03T14:54:51Z

components/table/index.css

+	&.is-selected {
+		.spectrum-Table-cell {
+			/* Remove bottom border by default for all selected rows to conditionally add it back. */
+			border-block-end: none;
+		}
+	}
+
+	/* Adding the bottom border only to the last selected row in a sequence achieves 1px border between adjacent selected rows */
+	&.is-selected:not(:has(+ .is-selected)) .spectrum-Table-cell {
+		border-block-end: var(--mod-table-border-width, var(--spectrum-table-border-width)) solid var(--highcontrast-table-focus-indicator-color, var(--mod-table-focus-indicator-color, var(--spectrum-table-focus-indicator-color)));
+	}


I got some help from Cursor with this, so please review and let me know if there's red flags. I'm particularly concerned with the :not(:has()) situation. In order to test this, you'll have to add the is-selected class to an adjacent row.

Basically, this is the code to produce 1px of blue border, between adjacent selected rows. I noted in the PR description that during my talks with Stefan, he said design's preference is to have only 1px of border between adjacent rows. He did say though that he understands it's quite complicated, and had already discussed the possibility of needing 2px in between adjacent selected rows. Although the 2px isn't ideal, if we decide that's probably the best route long-term, it's acceptable.

What are our thoughts on this approach to achieving the single pixel border between adjacent selected rows? I do believe there would be 1px of content shift, so does that mean we should toss this idea out? I couldn't achieve only 1px between ALL adjacent rows, when some are selected and some are not, so maybe this approach is overkill, since there's 2px borders in between other rows anyways?

I'd love to pair on this with someone if there's more ideas I haven't tried or to explain any of that ☝️ more!

hmm, yea this is always tricky. I am seeing the 1px shift when adding and removing selected state. I also noticed in the mockup it looks like the row is retaining its top borders. Not sure if that was an oversight or if its intentional. But if it is we could probably try using something like box-shadow: inset 0 -1px 0 var(--highcontrast-table-focus-indicator-color, var(--mod-table-focus-indicator-color, var(--spectrum-table-focus-indicator-color))); here instead of the border. Using insets my fix the shifting issue. In the end it might come down to whether or not design is okay with a content shift or the 2px border.

Stefan recently created a bunch of mock ups for me, and we worked through some of this. None of his examples have the top & bottom border like this. There's some inconsistencies in Figma, so I would guess it is not intentional.

Let me mess around with the inset and box-shadow thing to see if I can fix the content shift. Stefan also said he was happy to look at the storybook link, so I may take him up on that and ask about this.

marissahuysentruyt · 2025-06-03T16:53:35Z

TODO: write the changeset (maybe I can add to the part 1 changeset?)

.changeset/petite-toys-greet.md

saashapina

This is looking good so far Marissa. I just had a couple initial questions and I took a look at the selected issue you were seeing.

saashapina · 2025-06-06T12:21:27Z

components/table/index.css

-		--highcontrast-table-row-text-color-hover: HighlightText;
+	/* Row States */
+	/* @todo Refactor or remove these properties once the RGB stripped value is available. */
+	--spectrum-table-row-background-color-hover: rgb(var(--spectrum-gray-900-rgb), var(--spectrum-table-row-hover-opacity));


Will this eventually use a --spectrum-table-row-hover-color token instead of --spectrum-gray-900-rgb token like the spec?

That's a good question, and the only answer I have is maybe? 🤷‍♀️

There's a few instances where we've had to run the component tokens back to a more "basic" or primitive token like this. I think it's difficult because of how the postcss plugin extracts the properties and recreates them using rgb() functions. I wish the hover and down tokens were set to the rgb values, sort of like the selected row background colors were.

--spectrum-table-row-hover-color is equal to var(--spectrum-gray-900)- only the custom property.
--spectrum-gray-900 is equal to rgb(var(--spectrum-gray-900-rgb)), specifically with the rgb function

So when I was trying to use both tokens, it doesn't render, maybe because there's 2 rgb functions in a row? I don't really know. When I set:
--spectrum-table-row-background-color-hover: rgb(var(--spectrum-table-row-hover-color), var(--spectrum-table-row-hover-opacity))

it resolves to this, and the color doesn't render in any of my browsers.

saashapina · 2025-06-06T12:21:34Z

components/table/index.css

+	/* Row States */
+	/* @todo Refactor or remove these properties once the RGB stripped value is available. */
+	--spectrum-table-row-background-color-hover: rgb(var(--spectrum-gray-900-rgb), var(--spectrum-table-row-hover-opacity));
+	--spectrum-table-row-background-color-active: rgb(var(--spectrum-gray-900-rgb), var(--spectrum-table-row-down-opacity)); /* active/down state background color */


Do we need this extra comment for the active background color here?

Personally, I think it's ok to have the single comment for both of these properties. There's a chunk of similar custom props farther up in the file that have the same comment.

components/table/index.css

saashapina · 2025-06-06T13:54:52Z

components/table/index.css

+	&.is-selected {
+		.spectrum-Table-cell {
+			/* Remove bottom border by default for all selected rows to conditionally add it back. */
+			border-block-end: none;
+		}
+	}
+
+	/* Adding the bottom border only to the last selected row in a sequence achieves 1px border between adjacent selected rows */
+	&.is-selected:not(:has(+ .is-selected)) .spectrum-Table-cell {
+		border-block-end: var(--mod-table-border-width, var(--spectrum-table-border-width)) solid var(--highcontrast-table-focus-indicator-color, var(--mod-table-focus-indicator-color, var(--spectrum-table-focus-indicator-color)));
+	}


hmm, yea this is always tricky. I am seeing the 1px shift when adding and removing selected state. I also noticed in the mockup it looks like the row is retaining its top borders. Not sure if that was an oversight or if its intentional. But if it is we could probably try using something like box-shadow: inset 0 -1px 0 var(--highcontrast-table-focus-indicator-color, var(--mod-table-focus-indicator-color, var(--spectrum-table-focus-indicator-color))); here instead of the border. Using insets my fix the shifting issue. In the end it might come down to whether or not design is okay with a content shift or the 2px border.

github-actions · 2025-06-06T14:31:53Z

File metrics

Summary

Total size: 1.42 MB^*
Total change (Δ): 🔴 ⬆ 0.33 KB (0.02%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package	Size	Minified	Gzipped	Δ
table	47.21 KB	44.99 KB	5.33 KB	🔴 ⬆ 3.40 KB

File change details

table

Filename	Head	Minified	Gzipped	Compared to base
index.css	47.21 KB	44.99 KB	5.33 KB	🔴 ⬆ 3.40 KB
metadata.json	22.10 KB	-	-	🔴 ⬆ 1.54 KB

tokens

Filename	Head	Minified	Gzipped	Compared to base
css/dark-vars.css	46.24 KB	-	-	🔴 ⬆ 0.08 KB
css/global-vars.css	80.22 KB	-	-	-
css/index.css	259.43 KB	-	-	🔴 ⬆ 0.17 KB
css/large-vars.css	44.17 KB	-	-	-
css/light-vars.css	46.24 KB	-	-	🔴 ⬆ 0.08 KB
css/medium-vars.css	44.29 KB	-	-	-
json/tokens.json	410.29 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-06-06T14:32:44Z

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

components/table/stories/table.test.js

components/table/index.css

aramos-adobe · 2025-06-09T17:23:03Z

components/table/index.css

+/* Quiet table header rows do not have inline borders */
+.spectrum-Table--quiet .spectrum-Table-headRow:first-child {
+	.spectrum-Table-headCell:first-child {
+		border-inline-start: 0;
+	}
+
+	.spectrum-Table-headCell:last-child {
+		border-inline-end: 0;
+	}
+}
+
+/* Quiet table header rows do not have inline borders */
+.spectrum-Table--quiet .spectrum-Table-headRow:first-child {
+	.spectrum-Table-headCell:first-child {
+		border-inline-start: 0;
+	}
+
+	.spectrum-Table-headCell:last-child {
+		border-inline-end: 0;
+	}
+}


Duplicate selectors here

Thanks for catching that! It must have come over during a rebase 🥴

rise-erpelding

This looks great! I did check all the color tokens and they all appear to be matching the spec!

I can see that one down side to breaking the migration up like this is that there are certain things that were updated in the code in the previous PR that I can't comment on inline in GitHub because they weren't updated in this PR, but became more relevant as you were working with colors.

One example is this, about the sort Button:
I believe it was Aziz who pointed out that there were particularly some issues with the button in WHCM, colors that aren't behaving as intended. I notice that the variant of button that we're using is the default one, if I remove all the button passthroughs:

It's honestly probably fine as it is, but we might encounter less friction if the variant of button looked more like the design, for instance with the secondary button:

When I was working on Tag Group earlier, I noticed that the variant of action button was specified in the design (Quiet), which I think I'd probably want to surface in documentation so that consumers know that it would be ideal to use the same variant. Wondering if it's a good idea to do something like that here? It may not be if we're already overriding every style.

Within the testing preview, correct me if I'm wrong, but I don't think I noticed any hover states being tested there. That could be a nice addition.

Also within the realm of things that weren't updated in this PR, I noticed a few oddities when I was glancing through the CSS file:

We have a place around L352 where we're transitioning the header row color but I don't think the color of the header row ever changes, so it doesn't seem to make a lot of sense to have a transition here (Cursor agreed with me): transition: color var(--highcontrast-table-transition-duration, var(--mod-table-transition-duration, var(--spectrum-table-transition-duration))) ease-in-out;.
There was also a place where --highcontrast-table-transition-duration is set to 0, I think that was from before, but I wasn't able to figure out what that does. It might have a totally valid purpose though!
We have a couple of places starting around L906 where there's a comma in the CSS custom property transform: var(--spectrum-logical-rotation,);, I don't think this is hurting anything except my desire for perfect CSS punctuation.

A couple of other questions I had:

Are drop targets a variant, similar to collapsible, that aren't explicitly in S2? The borders there are a little funny, but not worth fixing if they're not in the spec. What is the plan for those variants that aren't in S2 but we haven't removed? Did design have an opinion on that?
For the scrollable + emphasized table, is the missing left/start side border intentional? I can see how having it there would look imbalanced if the scroll bar is on the other side, and I think it only looks off to me because I see it in comparison to the other variants, it would probably look better on its own not next to non-scrolling variants.

components/table/index.css

.changeset/petite-toys-greet.md

components/table/stories/table.test.js

components/table/stories/template.js

marissahuysentruyt · 2025-06-10T22:54:45Z

components/table/index.css

@@ -299,7 +349,6 @@
 		background-color: var(--mod-table-header-background-color, var(--spectrum-table-header-background-color));
 		border-block-start: var(--mod-table-border-width, var(--spectrum-table-border-width)) solid var(--highcontrast-table-border-color, var(--mod-table-border-color, var(--spectrum-table-border-color)));

-		transition: color var(--highcontrast-table-transition-duration, var(--mod-table-transition-duration, var(--spectrum-table-transition-duration))) ease-in-out;


@rise-erpelding This should address your question about whether we need this- no we shouldn't, since the header cells don't have any transition. This was probably left over from part 1, because I know I had header cell styles like this (I read the token specs wrong) but then removed them. Good catch!

Also, about this --highcontrast-table-transition-duration. I believe this is probably set to 0 so that there's not any animation for it. For instance, when a collapsed row expands, in WHCM it would just pop right open, instead of slide/transform. I can't replicate that behavior though, so it's sort of a guess.

components/table/index.css

marissahuysentruyt · 2025-06-10T23:23:27Z

@rise-erpelding

Are drop targets a variant, similar to collapsible, that aren't explicitly in S2? The borders there are a little funny, but not worth fixing if they're not in the spec. What is the plan for those variants that aren't in S2 but we haven't removed? Did design have an opinion on that?

Drop targets are a variant that aren't explicitly in S2, and they are not in the spec. I can make a card to revisit the borders (I think I saw the body dropzone is the funkiest, maybe?). When I was talking with Stefan about the dropzones, he referenced React's drag and drop functionality, and that it is currently "beta." Stefan suggested that we could treat our dropzones the same. Design isn't certain if they will work on dropzone/drag and drop stuff specifically, so they don't have a time frame, but it is something they've explored and want to come back to in the future.

components/table/index.css

rise-erpelding

I think this just needs a few little changes that I found places for within the comments! I'm going to double-check that I addressed all your questions as well.

.changeset/petite-toys-greet.md

components/table/index.css

tokens/custom/dark-vars.css

components/table/stories/template.js

components/table/stories/table.test.js

rise-erpelding

Crushed it!

* feat(tokens): adds light/dark custom vars for selected row * chore(tokens): create changeset * docs(table): update stories and template - add focused cell to table states story - add sparkline chart story - adds hasChartContent arg to story file and story - handles hasChartContent in template markup - add sparkline svgs - add sparkline test case - correct wording in stories - adds hover state to table rows states test - updates table rows states story - updates cellCustomClasses to use is-focus-visible for focused cells - adds isHovered arg to template - updates isFocused arg to apply is-focus-visible class instead of is-focused * test(table): add hover state to table rows states test * feat(table): update tokens to s2 colors - updates row colors for header row, summary, section header, and selected row - updates sort icon colors - updates disclosure/menu icons - fixes checkbox indeterminate icon color - adds .spectrum-Table-headCell--alignEnd class - selected rows have a 1px border around all cells in blue - row focus should be indicated by the side-focus-indicator regardless of selection state. this means a focused+unselected row will have a 1px gray border around cells, while a focused+selected row will have a 1px blue border around cells. - adjacent selected rows have a 1px border between them in blue (avoids duplicate borders between cells) - adds support for CJK languages for line-height - adds missing font tokens for header and body row text - remove quiet table cell background colors lines - there's not really color changes specc'd out for the header table cell background states, so header table cell background states are removed - quiet and scroller styles fixed up - in focused unselected rows, the text color was the same as the background color. This is now fixed, and should also distinguish the focused+selected rows from the focused+unselected rows as well. - add missing mod from documentation - add sparkline chart styles - opts for border on the ::after pesudo element for the button's focus indicator - styles the button so it fills the header cell's space - corrects the icon colors in the head cells - fixes background colors, text colors, icon colors, interactive states, and border radius in high contrast mode for the button in the head cell - removes duplicate quiet styles - adds forced-color-adjust: none to avoid flash of orange button text color in WHCM - update cell borders in whcm - adds table mods to button passthroughs for fallbacks - removes transition on header cells - removes unnecessary commas in some property definitions - define summary row text color in WHCM - removes is-focused class from table rows and cells in favor of using :focus-visible and :focus pseudo-classes only. * chore(table): update metadata * chore(table): edit changesets - correct syntax in part 1 table changeset (fixes stray quote) - creates separate changeset for part 2 table migration only

marissahuysentruyt self-assigned this May 21, 2025

marissahuysentruyt added size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. wip This is a work in progress, don't judge. skip_vrt Add to a PR to skip running VRT (but still pass the action) S2 Spectrum 2 labels May 21, 2025

marissahuysentruyt force-pushed the marissahuysentruyt/css-1178-s2-table-migration-layout-pt1 branch 2 times, most recently from 36920dd to 490a4e9 Compare May 21, 2025 22:18

marissahuysentruyt mentioned this pull request May 22, 2025

feat(table): s2 table migration layout part-1 #3799

Merged

35 tasks

marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch 2 times, most recently from 57cdc81 to f9f4cd3 Compare May 27, 2025 22:16

marissahuysentruyt force-pushed the marissahuysentruyt/css-1178-s2-table-migration-layout-pt1 branch from 4b76b0c to 85d324a Compare May 28, 2025 13:54

marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch 2 times, most recently from 58bd149 to 870191b Compare May 29, 2025 19:39

marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch from 870191b to 73ee64e Compare June 2, 2025 15:43

marissahuysentruyt force-pushed the marissahuysentruyt/css-1178-s2-table-migration-layout-pt1 branch from b746cdf to 1641315 Compare June 2, 2025 15:48

marissahuysentruyt marked this pull request as ready for review June 3, 2025 12:10

marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch 2 times, most recently from 41a351d to e8478ca Compare June 3, 2025 14:36

marissahuysentruyt commented Jun 3, 2025

View reviewed changes

tokens/custom/dark-vars.css Show resolved Hide resolved

marissahuysentruyt commented Jun 3, 2025

View reviewed changes

marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch from e8478ca to 64338cd Compare June 3, 2025 14:55

marissahuysentruyt added ready-for-review and removed wip This is a work in progress, don't judge. labels Jun 3, 2025

marissahuysentruyt force-pushed the marissahuysentruyt/css-1178-s2-table-migration-layout-pt1 branch from 781d447 to 6cf3b05 Compare June 3, 2025 18:36

marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch from 64338cd to 1462fcf Compare June 4, 2025 13:28

marissahuysentruyt requested a review from rise-erpelding June 4, 2025 21:06

Base automatically changed from marissahuysentruyt/css-1178-s2-table-migration-layout-pt1 to spectrum-two June 5, 2025 21:51

marissahuysentruyt commented Jun 5, 2025

View reviewed changes

.changeset/petite-toys-greet.md Outdated Show resolved Hide resolved

saashapina reviewed Jun 6, 2025

View reviewed changes

marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch from 1462fcf to 05e906d Compare June 6, 2025 14:27

marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch from d8274d7 to ddc7b29 Compare June 6, 2025 18:29

aramos-adobe reviewed Jun 9, 2025

View reviewed changes

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

aramos-adobe reviewed Jun 9, 2025

View reviewed changes

components/table/index.css Show resolved Hide resolved

aramos-adobe reviewed Jun 9, 2025

View reviewed changes

rise-erpelding reviewed Jun 10, 2025

View reviewed changes

components/table/index.css Show resolved Hide resolved

.changeset/petite-toys-greet.md Show resolved Hide resolved

marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch from ddc7b29 to 70dcf8c Compare June 10, 2025 22:06