fix(table): update tokens and add E2E themed tests #9646

alisonailea · 2024-06-20T16:47:39Z

Related Issue: #7180

Summary

Add tokens for table

…-chip tokens

This reverts commit c1e7945.

alisonailea · 2024-06-21T01:14:27Z

packages/calcite-components/src/components/table-row/table-row.scss

@@ -5,11 +5,9 @@
 *
 * @prop --calcite-table-row-background: [Deprecated] Use `--calcite-table-row-background-color` instead. Specifies the background color of the component.
 * @prop --calcite-table-row-background-color: Specifies the background color of the component.
- * @prop --calcite-table-row-background-color-striped: The background color of the component's `striped` rows, when specified.
+ * @prop --calcite-table-row-secondary-background-color: The background color of the component's `striped` rows, when specified.


I discussed this token name change with Skye and Aaron. The use of the "secondary" group here aligns better with our naming schema.

Hm. This seems unexpected to me. The name corresponds directly to the property that causes it to display. We don’t really use secondary elsewhere.

“striped-background-color”?

primary / secondary is a common naming pattern for design system colors and one I think we'll be expanding on in the future. "striped" is very specific to table.

Yeah, I guess that's my take - it is specific to table. To this point we've been intentional to not use primary / secondary / tertiary, so teams can build their own meaning through our properties. I think that's a fine direction to head in if we choose to in the future, but beginning to add it in now seems strange - this css property can only be used when striped is used so I think it makes sense. Adding "secondary" verbiage as the only public-facing case of it here seems strange. Maybe a good discussion with the larger team.

does "--calcite-table-row-striped-secondary-background-color" work? That way at least there is some indication to a user it pertains and will only be visible when the striped property is in use.

I would agree with this. If the default row color is "--calcite-table-row-background-color", then I would expect the striped prop to correlate to a "--calcite-table-row-striped-background-color" css property. I don't think the word "secondary" is necessary especially since there is no need for a "--calcite-table-row-primary-background-color" for it to correlate to.

packages/calcite-components/src/components/table-row/table-row.scss

macandcheese · 2024-06-21T04:09:41Z

packages/calcite-components/src/components/table-cell/table-cell.scss

  );
 }

 .selection-cell.selected-cell {
-  box-shadow: inset 0.25rem 0 0 0 var(--calcite-table-row-selected-accent-color, var(--calcite-color-brand));
-  color: var(--calcite-table-selection-cell-icon-color-selected, var(--calcite-color-brand));
+  box-shadow: var(--calcite-table-cell-shadow-selected, inset 0.25rem 0 0 0 var(--calcite-color-brand));


I wonder if the accent terminology is more understandable to an end user. It’s really creating a border accent - like we have in alert, notice, etc. this terminology with shadow makes it seem like the shadow is customizable.

It’s also only applicable to the single selection cell that receives the accent so I think the prior naming makes more sense. It should only be set on the row once, and not on an individual component. The cell that receives this color override is rendered internally within the Table Row.

thanks for the catch! But I think naming wise it aligns more with the "indicator" pattern that exists in other components.

Yeah, I know Alert uses "accent" for a similar thing, I think we have "highlight" in use (or talked about it). I think as long as we don't use shadow here I'm ok with it.

Really in plain terms this is "the color of the "selected indicator" border (or, for the other props - the icon) in the selectable checkbox cell when that cell is selected" . Because the user can't target this Cell directly, I think using terminology that has Table Row makes sense, since it can be controlled once at the app / table parent level or still individually per row.

"--calcite-table-row-indicator-color-selected", "--calcite-table-row-selected-highlight/indicator", "--calcite-table-row-selection/selected-indicator-color" or some combination like those etc., would maybe solve both our concerns?

macandcheese · 2024-06-21T21:06:10Z

packages/calcite-components/src/components/table-cell/table-cell.scss

-  color: var(--calcite-table-cell-text-color-selected, var(--calcite-color-brand));
-  & calcite-icon {
-    --calcite-icon-color: var(--calcite-table-cell-text-color-selected, var(--calcite-color-brand));
+  box-shadow: inset 0.25rem 0 0 0 var(--calcite-table-cell-indicator-color-selected, var(--calcite-color-brand));


This doesn't address my issue with this naming change - this is really a row-specific visual display and I think we should keep that nomenclature. I still think it should use table-row terminology, and that "--calcite-table-row-selected-accent-color" is a more apt name. "selected-highlight, "selected-indicator") all work for me, as long as its scoped to the row.

macandcheese

I made some comments about nomenclature, etc., but I think there are some other changes that caused issues - namely moving the row styles around.

I'm seeing double border in most places and some un-set icon color for selection, etc.

It'd also be nice to update the table.html demo page themed examples with the new properties to ensure we aren't missing any of the combinations. Thank you for tackling this!

github-actions · 2024-07-02T01:48:33Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

github-actions · 2024-07-15T01:51:50Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

github-actions · 2024-08-23T01:51:38Z

This PR has been automatically marked as stale because it has not had recent activity. Please close your PR if it is no longer relevant. Thank you for your contributions.

jcfranco · 2024-09-15T06:04:16Z

Closing as we are tackling component tokens in smaller chunks. Thanks, everyone! ✨💪✨

alisonailea added 7 commits June 18, 2024 08:55

fix(pagination): add classes for better testing

c1e7945

fix(table-cell): add token fallbacks from table and table-row

3a98a0b

fix(table-row): update component tokens with fallbacks to table

a75d013

fix(table): remove unused component tokens and fix stateful selection…

95f1508

…-chip tokens

docs(table): alphabetize tokens

6baa79d

fix(table-row, table-header, table-cell): refactor component tokens

c4a3813

test(table, table-row, table-header, table-cell): add themed e2e tests

a816869

alisonailea changed the base branch from dev to epic/7180-component-tokens June 20, 2024 16:47

alisonailea added 2 commits June 20, 2024 17:29

test(table, table-row, table-header, table-cell): add themed e2e tests

3053343

fix(table-cell): update tokens

2300f14

alisonailea marked this pull request as ready for review June 21, 2024 01:08

alisonailea requested a review from a team as a code owner June 21, 2024 01:08

chore(pagination): revert pagination changes

787a552

This reverts commit c1e7945.

alisonailea commented Jun 21, 2024

View reviewed changes

packages/calcite-components/src/components/table-row/table-row.scss Show resolved Hide resolved

macandcheese reviewed Jun 21, 2024

View reviewed changes

fix(table-cell): cleanup indicator colors

9341dff

alisonailea requested review from macandcheese and a team June 21, 2024 20:10

macandcheese reviewed Jun 21, 2024

View reviewed changes

macandcheese requested changes Jun 21, 2024

View reviewed changes

github-actions bot added Stale Issues or pull requests that have not had recent activity. and removed Stale Issues or pull requests that have not had recent activity. labels Jul 2, 2024

github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Jul 15, 2024

alisonailea marked this pull request as draft August 14, 2024 22:42

github-actions bot removed the Stale Issues or pull requests that have not had recent activity. label Aug 16, 2024

github-actions bot added the Stale Issues or pull requests that have not had recent activity. label Aug 23, 2024

jcfranco closed this Sep 15, 2024

jcfranco deleted the astump/7180-table branch September 15, 2024 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(table): update tokens and add E2E themed tests #9646

fix(table): update tokens and add E2E themed tests #9646

alisonailea commented Jun 20, 2024

alisonailea Jun 21, 2024

macandcheese Jun 21, 2024

macandcheese Jun 21, 2024

alisonailea Jun 21, 2024

macandcheese Jun 21, 2024

macandcheese Jun 21, 2024

SkyeSeitz Jul 3, 2024

macandcheese Jun 21, 2024

macandcheese Jun 21, 2024

alisonailea Jun 21, 2024

macandcheese Jun 21, 2024 •

edited

Loading

macandcheese Jun 21, 2024

macandcheese left a comment •

edited

Loading

github-actions bot commented Jul 2, 2024

github-actions bot commented Jul 15, 2024

github-actions bot commented Aug 23, 2024

jcfranco commented Sep 15, 2024

fix(table): update tokens and add E2E themed tests #9646

fix(table): update tokens and add E2E themed tests #9646

Conversation

alisonailea commented Jun 20, 2024

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macandcheese Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macandcheese left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jul 2, 2024

github-actions bot commented Jul 15, 2024

github-actions bot commented Aug 23, 2024

jcfranco commented Sep 15, 2024

macandcheese Jun 21, 2024 •

edited

Loading

macandcheese left a comment •

edited

Loading