Skip to content
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.

Sign up for GitHub

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

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

Closed
wants to merge 11 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c1e7945
fix(pagination): add classes for better testing
alisonailea Jun 18, 2024
3a98a0b
fix(table-cell): add token fallbacks from table and table-row
alisonailea Jun 18, 2024
a75d013
fix(table-row): update component tokens with fallbacks to table
alisonailea Jun 18, 2024
95f1508
fix(table): remove unused component tokens and fix stateful selection…
alisonailea Jun 18, 2024
6baa79d
docs(table): alphabetize tokens
alisonailea Jun 19, 2024
c4a3813
fix(table-row, table-header, table-cell): refactor component tokens
alisonailea Jun 20, 2024
a816869
test(table, table-row, table-header, table-cell): add themed e2e tests
alisonailea Jun 20, 2024
3053343
test(table, table-row, table-header, table-cell): add themed e2e tests
alisonailea Jun 20, 2024
2300f14
fix(table-cell): update tokens
alisonailea Jun 21, 2024
787a552
chore(pagination): revert pagination changes
alisonailea Jun 21, 2024
9341dff
fix(table-cell): cleanup indicator colors
alisonailea Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions packages/calcite-components/src/components.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4562,6 +4562,10 @@ export namespace Components {
* Accessible name for the dropdown menu.
*/
"dropdownLabel": string;
/**
* Defines the available placements that can be used when a flip occurs.
*/
"flipPlacements": FlipPlacement[];
/**
* Specifies the kind of the component, which will apply to border and background, if applicable.
*/
Expand All @@ -4574,6 +4578,11 @@ export namespace Components {
* Determines the type of positioning to use for the overlaid content. Using `"absolute"` will work for most cases. The component will be positioned inside of overflowing parent containers and will affect the container's layout. `"fixed"` should be used to escape an overflowing parent container, or when the reference element's `position` CSS property is `"fixed"`.
*/
"overlayPositioning": OverlayPositioning;
/**
* Determines where the component will be positioned relative to the container element.
* @default "bottom-end"
*/
"placement": MenuPlacement;
/**
* Specifies an icon to display at the end of the primary button.
*/
Expand Down Expand Up @@ -12509,6 +12518,10 @@ declare namespace LocalJSX {
* Accessible name for the dropdown menu.
*/
"dropdownLabel"?: string;
/**
* Defines the available placements that can be used when a flip occurs.
*/
"flipPlacements"?: FlipPlacement[];
/**
* Specifies the kind of the component, which will apply to border and background, if applicable.
*/
Expand All @@ -12529,6 +12542,11 @@ declare namespace LocalJSX {
* Determines the type of positioning to use for the overlaid content. Using `"absolute"` will work for most cases. The component will be positioned inside of overflowing parent containers and will affect the container's layout. `"fixed"` should be used to escape an overflowing parent container, or when the reference element's `position` CSS property is `"fixed"`.
*/
"overlayPositioning"?: OverlayPositioning;
/**
* Determines where the component will be positioned relative to the container element.
* @default "bottom-end"
*/
"placement"?: MenuPlacement;
/**
* Specifies an icon to display at the end of the primary button.
*/
Expand Down
109 changes: 61 additions & 48 deletions packages/calcite-components/src/components/table-cell/table-cell.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,14 @@
*
* @prop --calcite-table-cell-background: [Deprecated] Use `--calcite-table-cell-background-color` instead. Specifies the background color of the component.
* @prop --calcite-table-cell-background-color: Specifies the background color of the component.
* @prop --calcite-table-cell-background-color-selected: Specifies the background color of the component in a selected row.
* @prop --calcite-table-cell-border-color: Specifies the border color of the component.
* @prop --calcite-table-cell-text-color: Specifies the text color of the component.
* @prop --calcite-table-cell-number-background-color: Specifies the background color of the number cell when "numbered" is set on the parent table.
* @prop --calcite-table-cell-footer-background-color: Specifies the background color of the footer cell.
* @prop --calcite-table-cell-text-color-selected: Specifies the text color of the component in a selected row.
* @prop --calcite-table-cell-indicator-color-highlight: Specifies the selection indicator color when selection-mode is set on the parent table.
* @prop --calcite-table-cell-indicator-color-selected: Specifies the selection indicator color of the component in a selected row.
*/

/**
Expand All @@ -15,6 +22,14 @@
* --calcite-internal-table-cell-background-color
*/

/**
* CSS Custom Properties
*
* These properties can be overridden using the component's tag as selector.
*
* @prop --calcite-table-cell-background: [Deprecated] Use `--calcite-table-cell-background-color` instead. Specifies the background color of the component.
*/

:host {
@apply contents;
}
Expand All @@ -32,15 +47,23 @@
}

td {
@apply text-start align-middle text-color-1 whitespace-normal;
background-color: var(
--calcite-internal-table-cell-background-color,
var(--calcite-table-cell-background-color, transparent)
--calcite-internal-table-cell-background-color: var(
--calcite-table-cell-background-color,
var(--calcite-table-cell-background, transparent)
);
--calcite-internal-table-cell-text-color: var(--calcite-table-cell-text-color, var(--calcite-color-text-1));

@apply text-start align-middle whitespace-normal;
background-color: var(--calcite-internal-table-cell-background-color);
font-size: var(--calcite-internal-table-cell-font-size);
border-inline-end: 1px solid
var(--calcite-table-cell-border-color, var(--calcite-table-border-color, var(--calcite-color-border-3)));
border-color: var(
--calcite-table-cell-border-color,
var(--calcite-table-row-border-color, var(--calcite-table-border-color, var(--calcite-color-border-3)))
);
border-style: solid;
border-inline-end-width: var(--calcite-border-width-sm);
padding: var(--calcite-internal-table-cell-padding);
color: var(--calcite-internal-table-cell-text-color);

&:not(.static-cell) {
@apply focus-base;
Expand All @@ -62,76 +85,66 @@ td.last-cell {
border-inline-end: 0;
}

.number-cell {
--calcite-internal-table-cell-background-color: var(
--calcite-table-cell-number-background-color,
var(--calcite-color-foreground-2)
);
}

.footer-cell {
@apply font-medium;
color: var(--calcite-color-text-1);
background-color: var(
--calcite-table-cell-background-color,
var(--calcite-table-cell-background, var(--calcite-color-background))
--calcite-internal-table-cell-background-color: var(
--calcite-table-cell-footer-background-color,
var(--calcite-color-background)
);

border-block-start: 1px solid
var(--calcite-table-cell-border-color, var(--calcite-table-border-color, var(--calcite-color-border-3)));
@apply font-medium;
border-block-start-width: var(--calcite-border-width-sm);
}

.number-cell,
.selection-cell {
@apply text-center;
border-inline-end: 1px solid
var(--calcite-table-cell-border-color, var(--calcite-table-border-color, var(--calcite-color-border-3)));

inline-size: 2rem;
min-inline-size: 2rem;
}

.number-cell {
color: var(--calcite-table-number-cell-text-color, var(--calcite-color-text-1));
background-color: var(--calcite-table-number-cell-background-color, var(--calcite-color-foreground-2));
&.footer-cell {
background-color: var(
--calcite-table-number-cell-background-color,
var(--calcite-table-cell-background-color, var(--calcite-color-background))
);
}
}

.selection-cell {
color: var(--calcite-table-selection-cell-icon-color, var(--calcite-color-text-3));
@apply align-middle;
inset-inline-start: 2rem;

&:not(.footer-cell) {
@apply cursor-pointer;
background-color: var(
--calcite-table-selection-cell-background-color,
var(--calcite-table-cell-background-color, transparent)
);
}
&.footer-cell {
background-color: var(
--calcite-table-selection-cell-background-color,
var(--calcite-table-cell-background-color, var(--calcite-color-background))
);

calcite-icon,
::slotted(calcite-icon) {
--calcite-icon-color: var(--calcite-table-cell-indicator-color-highlight, var(--calcite-color-text-3));
}

::slotted(calcite-icon) {
@apply pointer-events-none mt-1;
}
}

.selected-cell:not(.number-cell):not(.footer-cell) {
background-color: var(
--calcite-internal-table-cell-background-color: var(
--calcite-table-cell-background-color-selected,
var(--calcite-table-row-background-color-selected, var(--calcite-color-foreground-current))
var(--calcite-color-foreground-current)
);
}

.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: inset 0.25rem 0 0 0 var(--calcite-table-cell-indicator-color-selected, var(--calcite-color-brand));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


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

.selection-cell {
@apply align-middle;
& ::slotted(calcite-icon) {
@apply pointer-events-none mt-1;
}
.calcite--rtl.selection-cell.selected-cell {
box-shadow: inset -0.25rem 0 0 0 var(--calcite-table-cell-indicator-color-selected, var(--calcite-color-brand));
}

@include disabled();
8 changes: 4 additions & 4 deletions packages/calcite-components/src/components/table-header/table-header.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* @prop --calcite-table-header-background: [Deprecated] Use the `--calcite-table-header-background-color` property instead. Specifies the background color of the component.
* @prop --calcite-table-header-background-color: Specifies the background color of the component.
* @prop --calcite-table-header-border-color: Specifies the border color of the component.
* @prop --calcite-table-header-heading-color: Specifies the heading color of the component.
* @prop --calcite-table-header-description-color: Specifies the description color of the component.
* @prop --calcite-table-header-heading-text-color: Specifies the heading color of the component.
* @prop --calcite-table-header-description-text-color: Specifies the description color of the component.

*/

Expand Down Expand Up @@ -113,10 +113,10 @@ th.last-cell {
}

.heading {
color: var(--calcite-table-header-heading-color, var(--calcite-color-text-1));
color: var(--calcite-table-header-heading-text-color, var(--calcite-color-text-1));
}

.description {
color: var(--calcite-table-header-description-color, var(--calcite-color-text-3));
color: var(--calcite-table-header-description-text-color, var(--calcite-color-text-3));
font-size: var(--calcite-internal-table-cell-font-size-secondary);
}
55 changes: 40 additions & 15 deletions packages/calcite-components/src/components/table-row/table-row.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

“striped-background-color”?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

* @prop --calcite-table-row-background-color-selected: The background color of the component's `selected` rows, when specified.
* @prop --calcite-table-row-border-color: Specifies the border color of the component.
* @prop --calcite-table-row-selected-accent-color: Specifies the border color of the component.
alisonailea marked this conversation as resolved.
Show resolved Hide resolved

*/

:host {
Expand All @@ -24,25 +22,52 @@
}
}

calcite-table-cell {
--calcite-internal-table-cell-background-color: var(
--calcite-table-row-background-color,
tr {
--calcite-internal-table-row-default-background-color: var(
--calcite-table-row-background,
var(--calcite-color-foreground-1)
);

border-block-end: 1px solid
var(--calcite-table-row-border-color, var(--calcite-internal-table-row-border-color, transparent));
background-color: var(
--calcite-table-row-background-color,
var(--calcite-internal-table-row-default-background-color)
);
}

tr.last-visible-row {
border-block-end: 0;
}

:host([selected]) calcite-table-cell {
--calcite-internal-table-cell-background-color: var(
:host([selected]) tr {
background-color: var(
--calcite-table-row-background-color-selected,
var(--calcite-color-foreground-1)
var(--calcite-table-row-background-color, var(--calcite-internal-table-row-default-background-color))
);
}

tr {
border-block-end: 1px solid var(--calcite-table-row-border-color, transparent);
background-color: var(--calcite-table-row-background-color, var(--calcite-color-foreground-1));
}
:host(:nth-child(2n + 1)) {
&:host([selected]) tr {
background-color: var(
--calcite-table-row-background-color-selected,
var(
--calcite-table-row-secondary-background-color,
var(
--calcite-internal-table-secondary-background-color,
var(--calcite-table-row-background-color, var(--calcite-internal-table-row-default-background-color))
)
)
);
}

tr.last-visible-row {
border-block-end: 0;
tr {
background-color: var(
--calcite-table-row-secondary-background-color,
var(
--calcite-internal-table-secondary-background-color,
var(--calcite-table-row-background-color, var(--calcite-internal-table-row-default-background-color))
)
);
}
}
Loading