Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: spectrum-two
Choose a base branch
from

Conversation

marissahuysentruyt
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt commented May 21, 2025

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:

  • text colors for column headers
  • text colors for "regular" rows
  • icon colors in "regular" rows
  • sort icon (3) colors in column headers
  • disclosure icon colors in column headers
  • background colors for headers and "regular" rows
  • summary row background color
  • section header background color
  • "regular" row hover and active background colors
    • Table rows are not clickable by default. Javascript is usually added to make them clickable, so the reviewer may have to toggle the active state to verify the table row down state token
  • selected row background color and border
    • Stefan confirmed the 1px blue border goes around all cells in a selected row.
  • selected row hover and down background colors
  • rounded corners on a cell's focus indicator
  • row's side focus indicator
    • The side focus indicator should always appear when a row is focused, regardless of its selection status. Stefan confirmed that the 1px blue border around the other cells in a selected row remains when a row is selected AND focused. For an unselected row that is focused, the default gray borders should render.
  • There's a new "with chart content" story. This demonstrates the 4 variants of the sparkline charts found in the desktop 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:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. 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. ✨

@marissahuysentruyt marissahuysentruyt self-assigned this May 21, 2025
@marissahuysentruyt 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
Copy link

changeset-bot bot commented May 21, 2025

⚠️ No Changeset found

Latest commit: ddc7b29

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes changesets to release 1 package
Name Type
@spectrum-css/table Major

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@marissahuysentruyt 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 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 marissahuysentruyt force-pushed the marissahuysentruyt/css-1178-s2-table-migration-layout-pt1 branch from 4b76b0c to 85d324a Compare May 28, 2025 13:54
@marissahuysentruyt 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
Copy link
Collaborator Author

marissahuysentruyt commented May 29, 2025

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. ✅
Screenshot 2025-06-03 at 10 43 50 AM

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. ✅
Screenshot 2025-05-29 at 5 03 05 PM

@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch from 870191b to 73ee64e Compare June 2, 2025 15:43
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1178-s2-table-migration-layout-pt1 branch from b746cdf to 1641315 Compare June 2, 2025 15:48
@marissahuysentruyt marissahuysentruyt marked this pull request as ready for review June 3, 2025 12:10
@marissahuysentruyt 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
@@ -57,6 +57,8 @@

--spectrum-steplist-current-marker-color-key-focus: var(--spectrum-blue-700);

--spectrum-table-selected-row-background-color-rgb: var(--spectrum-blue-800-rgb);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed in Figma that the color for light and dark modes do vary just a bit.

Comment on lines -705 to -712
&.spectrum-Table--quiet {
border-block-start: none;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +595 to +654
&.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)));
}
Copy link
Collaborator Author

@marissahuysentruyt marissahuysentruyt Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.
Screenshot 2025-06-06 at 9 40 01 AM

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stefan-2
stefan

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 marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch from e8478ca to 64338cd Compare June 3, 2025 14:55
@marissahuysentruyt
Copy link
Collaborator Author

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

@marissahuysentruyt marissahuysentruyt added ready-for-review and removed wip This is a work in progress, don't judge. labels Jun 3, 2025
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1178-s2-table-migration-layout-pt1 branch from 781d447 to 6cf3b05 Compare June 3, 2025 18:36
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch from 64338cd to 1462fcf Compare June 4, 2025 13:28
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1178-s2-table-migration-layout-pt1 branch 2 times, most recently from 5f0b680 to 101b69b Compare June 5, 2025 21:13
@marissahuysentruyt marissahuysentruyt requested review from saashapina and removed request for 5t3ph and saashapina June 5, 2025 21:16
Comment on lines 26 to 28
...isEmphasized,
description: "Emphasized styling on the table affects colors of selected rows and any checkboxes."
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: is this control necessary for all stories? Is it ok if we disable the control per story? Now that colors are in place, revisit some of the questions Rise posed in #3799

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

@saashapina saashapina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

--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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Screenshot 2025-06-06 at 10 46 12 AM

/* 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 */
Copy link
Collaborator

@saashapina saashapina Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +595 to +654
&.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)));
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.
Screenshot 2025-06-06 at 9 40 01 AM

- 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.
- adds hasChartContent arg to story file and story
- handles hasChartContent in template markup
- add sparkline svgs
- add sparkline test case
- 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
@marissahuysentruyt marissahuysentruyt force-pushed the marissahuysentruyt/css-1179-s2-table-migration-layout-pt2 branch from 1462fcf to 05e906d Compare June 6, 2025 14:27
Copy link
Contributor

github-actions bot commented Jun 6, 2025

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 46.79 KB 44.61 KB 5.41 KB 🔴 ⬆ 2.99 KB

File change details

table

Filename Head Minified Gzipped Compared to base
index.css 46.79 KB 44.61 KB 5.41 KB 🔴 ⬆ 2.99 KB
metadata.json 22.65 KB - - 🔴 ⬆ 2.10 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.

Copy link
Contributor

github-actions bot commented Jun 6, 2025

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sortable columns and head cell with menu button in WHCM:

  • The lead icon doesn't show until you hover over it
  • The sort icon needs a high contrast color, something to match the content of the button
  • Do we need the background color here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-06-09 at 10 47 09 AM Screenshot 2025-06-09 at 10 41 23 AM

@@ -220,26 +254,22 @@

.spectrum-Table-headCell {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In regards to my test comment on sortable and head cell with menu button and it's just a suggestion:

    --highcontrast-button-background-color-default: Canvas;
    --highcontrast-button-content-color-default: CanvasText;
    --highcontrast-button-background-color-hover: Canvas;
    --highcontrast-button-content-color-hover: Highlight;
    border: none;

Keeps focus on the table rows that are selected and that may have checkboxes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-06-09 at 11 35 58 AM Screenshot 2025-06-09 at 11 35 52 AM

Comment on lines +393 to +413
/* 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;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate selectors here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review S2 Spectrum 2 size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants