-
Notifications
You must be signed in to change notification settings - Fork 202
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
base: spectrum-two
Are you sure you want to change the base?
feat(table): migrates remaining table styles to s2 part2 #3818
Conversation
|
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
36920dd
to
490a4e9
Compare
57cdc81
to
f9f4cd3
Compare
4b76b0c
to
85d324a
Compare
58bd149
to
870191b
Compare
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: 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: EDIT: This was a transparent background color before, but with the updates to the header cell background colors, this should be taken care of. ✅ |
870191b
to
73ee64e
Compare
b746cdf
to
1641315
Compare
41a351d
to
e8478ca
Compare
@@ -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); |
There was a problem hiding this comment.
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.
&.spectrum-Table--quiet { | ||
border-block-start: none; | ||
} |
There was a problem hiding this comment.
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.
&.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))); | ||
} |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
e8478ca
to
64338cd
Compare
TODO: write the changeset (maybe I can add to the part 1 changeset?) |
781d447
to
6cf3b05
Compare
64338cd
to
1462fcf
Compare
5f0b680
to
101b69b
Compare
...isEmphasized, | ||
description: "Emphasized styling on the table affects colors of selected rows and any checkboxes." | ||
}, |
There was a problem hiding this comment.
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
There was a problem hiding this 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)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
/* 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 */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
&.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))); | ||
} |
There was a problem hiding this comment.
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.
- 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
1462fcf
to
05e906d
Compare
File metricsSummaryTotal size: 1.42 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
File change detailstable
tokens
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-3818--spectrum-css.netlify.app |
d8274d7
to
ddc7b29
Compare
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -220,26 +254,22 @@ | |||
|
|||
.spectrum-Table-headCell { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* 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; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate selectors here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -131,11 +140,17 @@ | |||
--mod-button-edge-to-text: var(--spectrum-table-cell-inline-spacing); | |||
--mod-button-padding-label-to-icon: var(--spectrum-table-visual-to-text); | |||
--mod-button-height: var(--spectrum-table-min-header-row-height); | |||
--mod-button-focus-ring-border-radius: var(--spectrum-table-focused-cell-border-radius); | |||
--mod-button-focus-ring-border-radius: calc(var(--mod-table-border-radius, var(--spectrum-table-focused-cell-border-radius)) - var(--mod-table-border-width, var(--spectrum-table-border-width))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't the ideal place for this comment but:
I'm wondering what your thoughts are about the passthroughs here, I've noticed that if you set --mod-table-header-text-color
just as one example, that it doesn't go through to the button.
I know we've been talking about how much longer we'll have mods so I'm always hesitant to make suggestions around them, but what are your thoughts on setting the passthroughs to be something like this?
--mod-button-content-color-default: var(--mod-table-header-text-color, var(--spectrum-table-header-text-color));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed there's a PR for the pre release of Table part 1 in #3907, any idea what happens with future releases if that merges before this one is done? I thought it was the changeset file that triggered the release PR, will it still pick up the correct changes if we edit an existing changeset file that's already had a release?
Description
This PR is part 2 of the table's migration to S2! Table has been updated to include:
Colors
&Typography
have been addressed)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
Regression testing
Validate:
Screenshots
To-do list