feat(tile): add design tokens #10476

eriklharper · 2024-10-02T21:48:49Z

Related Issue: #7180

Summary

Adds design tokens for the Tile component.

--calcite-tile-background-color: Specifies the background color of the component.
--calcite-tile-border-color: Specifies the border color of the component.
--calcite-tile-description-text-color: Specifies the description text color of the component.
--calcite-tile-heading-text-color: Specifies the heading text color of the component.
--calcite-tile-icon-color: Specifies the icon color in the component.
--calcite-tile-selection-icon-color: Specifies the color of the selection icon in the component.
--calcite-tile-selection-icon-color-hover: Specifies the color of the selection icon in the component on hover.
--calcite-tile-shadow: Specifies the box-shadow of the component.

packages/calcite-components/src/components/tile/tile.scss

alisonailea

Thanks Erik! Looking good. I have a few suggestions to ensure we keep the component aligned to current design patterns.

@ashetland can you confirm "heading" "description/text" align with the future cross-component heading patterns?

alisonailea · 2024-10-03T17:49:59Z

packages/calcite-components/src/components/tile/tile.scss

+    var(--calcite-color-text-3)
+  );
+  --calcite-internal-tile-heading-text-color: var(--calcite-tile-heading-text-color, var(--calcite-color-text-2));
+  --calcite-internal-tile-icon-color: var(--calcite-color-text-3);


it's confusing to set all the -internal variable defaults here. Rather defaults should be set as the final fallback on the style prop itself.

alisonailea · 2024-10-03T18:05:20Z

packages/calcite-components/src/components/tile/tile.scss

@@ -7,42 +7,65 @@
 * @prop --calcite-tile-border-color: Specifies the border color of the component.
 * @prop --calcite-tile-description-text-color: Specifies the description text color of the component.
 * @prop --calcite-tile-heading-text-color: Specifies the heading text color of the component.
+ * @prop --calcite-tile-icon-color: Specifies the icon color in the component.
+ * @prop --calcite-tile-selection-icon-color: Specifies the color of the selection icon in the component.
+ * @prop --calcite-tile-selection-icon-color-hover: Specifies the color of the selection icon in the component on hover.


suggestion:
replace all three icon tokens and the "tile-description" with
--calcite-tile-text-color and --calcite-tile-color-selected which covers the states and retains the overall design pattern

How would a user override the icon color separately from the title or description text? Just --calcite-icon-color? Wouldn't it be more expected to use --calcite-tile-icon-color for the display icon?

Because Tile now uses an actual calcite-icon for the radio and checkbox UI, the meaning of "icon" easily generates confusion too because there's the "selection" icon (radio or checkbox) and the "content" icon that displays above the heading. @macandcheese which icon are you referring to here?

The content one. I thought the original set you had here made the most sense, just IMO - to your point it makes the distinction between selection icon more clear.

Good point.
Per some other discussions, like a more general "close-button" pattern I think there is a more global "selection-icon" pattern that we could use. As for the tile-icon, what about something like the following?

calcite-icon { color: var(--calcite-title-icon-color, var(--calcite-icon-color, currentColor)); }

Yeah that looks good. I think the “selection-“ prefix does make it kind of clear and we use it elsewhere, so agreed with some consistent naming there it’s not as bad.

I don’t think using component specific naming is an issue there - even when we have the global overrides I think we’d want publicly documented component-specific ones.

.selection-icon-example-class { color: var(--calcite-title-selection-icon-color, var(--calcite-selection-icon-color, “whatever underlying default”)); }

Having these at the component level even if there are underlying higher level props makes documentation easier as well IMO. Users won’t need to track down these possible properties.

Agreed!

.selection-icon-example-class { color: var(--calcite-title-selection-icon-color, var(--calcite-selection-icon-color, “whatever underlying default”)); }

looks good.

Am I reading that right? Should it be --calcite-tile-icon-color ?

packages/calcite-components/src/components/tile/tile.scss

alisonailea · 2024-10-03T18:11:11Z

packages/calcite-components/src/components/tile/tile.scss

@@ -189,6 +218,8 @@
 :host([selection-appearance="border"][layout="horizontal"]),
 :host([selection-appearance="border"][layout="vertical"]) {
  .container.selected:focus::before {
+    --calcite-internal-tile-shadow: inset 0 0 0 1px var(--calcite-color-brand);


--calcite-internal-tile-shadow-color: var(--calcite-tile-color-selected, var(--calcite-color-brand));

alisonailea · 2024-10-03T18:12:10Z

packages/calcite-components/src/components/tile/tile.scss

@@ -202,21 +233,28 @@

 :host([selection-appearance="border"][layout="horizontal"]) {
  .container.selected {
-    box-shadow: inset 0 -4px 0 0 var(--calcite-color-brand);
+    --calcite-internal-tile-shadow: inset 0 -4px 0 0 var(--calcite-color-brand);


change this back to a static style prop

box-shadow: inset 0 -4px 0 0 var(--calcite-internal-tile-shadow-color, var(--calcite-color-brand));

alisonailea · 2024-10-03T18:19:01Z

packages/calcite-components/src/components/tile/tile.scss

  }
 }

 :host([selection-appearance="border"][layout="vertical"]) {
  .container.selected {
-    box-shadow: inset 4px 0 0 0 var(--calcite-color-brand);


only the "x" and "y" axis lengths are in changing between these box-shadows. You could probably simplify this down to a single style prop. Something like....

.container { box-shadow: inset var(--calcite-internal-tile-shadow-x) var(--calcite-internal-tile-shadow-y) 0 0 var(--calcite-internal-tile-shadow-color, none); } :host([selection-appearance="border"]) .container.selected { --calcite-internal-tile-shadow-color: var(--calcite-tile-color-selected, var(--calcite-color-brand)); } :host([layout="vertical"]) .container { --calcite-internal-tile-shadow-x: 4px; --calcite-internal-tile-shadow-y: 0; } :host([layout="horizontal"]) .container { --calcite-internal-tile-shadow-x: 0; --calcite-internal-tile-shadow-y: -4px; }

alisonailea · 2024-10-03T18:19:48Z

packages/calcite-components/src/components/tile/tile.scss

+  --calcite-internal-tile-border-color: var(--calcite-color-text-link);
+  --calcite-internal-tile-heading-text-color: var(--calcite-color-text-link);
+  --calcite-internal-tile-icon-color: var(--calcite-color-text-link);
+  --calcite-ui-icon-color: var(--calcite-color-text-link);


ui-icon-color is deprecated

Not sure if removing this would constitute a breaking change or not, so I hesitate to remove that. All of these changes straight-up copied over from the tokens branch, so my guess is this is here for a reason to prevent a breakage. Unless we're ok with breaking changes on these PRs, I suppose it would be fine to remove it. @jcfranco thoughts?

the --calcite-ui-icon-color is deprecated but still available on the calcite-icon however it is overwritten by --calcite-icon-color. However we don't just want to replace --calcite-ui-icon-color with --calcite-icon-color in this case because that's not how we're styling calcite-icon as a sub-component in other components.

Instead this should be something like the following.

calcite-icon { color: var(--calcite-tile-href-icon-color, var(--calcite-color-text-link)); }

alisonailea · 2024-10-03T18:23:54Z

packages/calcite-components/src/components/tile/tile.scss

-  --calcite-icon-color: var(--calcite-color-text-link);
+  --calcite-internal-tile-border-color: var(--calcite-color-text-link);
+  --calcite-internal-tile-heading-text-color: var(--calcite-color-text-link);
+  --calcite-internal-tile-icon-color: var(--calcite-color-text-link);


I think we need to add an extra token for this case --calcite-title-href-text-color

:host([href]:focus:not([disabled])), :host([href]:hover:not([disabled])) { .container, .heading { color: var(--calcite-title-href-text-color, var(--calcite-color-text-link)); } .container { border-color: var(--calcite-title-href-text-color, var(--calcite-color-text-link)); } }

…xt-color-hover" token to allow both a default and overridable hover color.

macandcheese · 2024-10-03T22:36:35Z

packages/calcite-components/src/components/tile/tile.scss

@@ -5,8 +5,9 @@
 *
 * @prop --calcite-tile-background-color: Specifies the background color of the component.
 * @prop --calcite-tile-border-color: Specifies the border color of the component.
- * @prop --calcite-tile-description-text-color: Specifies the description text color of the component.


Wouldn't we still want this? There are two different colors used for the heading / description UI ... I'd expect a css property to individually control each. cc @ashetland @SkyeSeitz to confirm.

@alisonailea 's suggestion is simply to replace --calcite-tile-description-text-color with --calcite-tile-text-color which effectively serves as like the "body" default text color, and because it is applied to the ~~.container div~~ host element instead of the .description div, it will set a color for the whole content of the tile, including icons and other content.

Description and heading are different colors currently and will remain different colors in future Tile enhancements.

Since there isn’t a default content slot, I’d expect the css props to match the property name for each.

Other content is slotted in. This is why I still think the “icon prop” would have its own css property.

As a user without knowledge of the component, setting the icon to the description color is confusing (or, having description / icon be text-color but then also having a “named” text color property for heading).

…ender this unnecessary and we don't want to ship a token we'll have to immediately deprecate

…so that one color token controls both the unfocused and focused states

…irectly on a calcite-tile element selector in CSS

… Restoring selection-appearance border selected styles with box-shadow rules.

…internal-tile-border-color and using brand token for hover, focus states on borders

…calcite-tile-heading-text-color-hover

…way of setting the color of the presentation calcite-icon using a direct color property, and adding additional --calcite-tile-href-icon-color-hover token that defaults to the link color

feat(tile): add design tokens

45d65d9

github-actions bot assigned eriklharper Oct 2, 2024

github-actions bot added the enhancement Issues tied to a new feature or request. label Oct 2, 2024

eriklharper and others added 2 commits October 2, 2024 15:48

updating storybook and demo pages for theming examples

c7c4f5e

Merge branch 'dev' into eriklharper/7180-tile-tokens

6d359d9

eriklharper marked this pull request as ready for review October 2, 2024 22:48

eriklharper requested review from benelan and jcfranco as code owners October 2, 2024 22:48

eriklharper requested review from alisonailea and jcfranco and removed request for jcfranco and benelan October 2, 2024 22:48

alisonailea reviewed Oct 3, 2024

View reviewed changes

packages/calcite-components/src/components/tile/tile.scss Outdated Show resolved Hide resolved

alisonailea requested changes Oct 3, 2024

View reviewed changes

replacing "description-text-color" with "text-color" and adding a "te…

b8e6d03

…xt-color-hover" token to allow both a default and overridable hover color.

macandcheese reviewed Oct 3, 2024

View reviewed changes

eriklharper added 3 commits October 3, 2024 16:59

removing --calcite-tile-text-color-hover since future patterns will r…

57a9261

…ender this unnecessary and we don't want to ship a token we'll have to immediately deprecate

injecting --calcite-tile-border-color into the tile's focus selector …

55dd86b

…so that one color token controls both the unfocused and focused states

removing --calcite-tile-shadow because user can just set box-shadow d…

6fdd1db

…irectly on a calcite-tile element selector in CSS

alisonailea mentioned this pull request Oct 7, 2024

Design token phase II - Integration across components #7180

Open

eriklharper added 6 commits October 8, 2024 11:30

adding --calcite-tile-shadow

aa92728

removing --calcite-internal-tile-shadow since it isn't used anywhere.…

0a90132

… Restoring selection-appearance border selected styles with box-shadow rules.

removing --calcite-internal-tile-outline-color in favor of --calcite-…

f8bb7a7

…internal-tile-border-color and using brand token for hover, focus states on borders

getting rid of --calcite-internal-tile-border-color

4f05f78

removing --calcite-internal-tile-heading-text-color and adding new --…

9718d7b

…calcite-tile-heading-text-color-hover

removing --calcite-internal-tile-icon-color, updating to use current …

a7a8ee7

…way of setting the color of the presentation calcite-icon using a direct color property, and adding additional --calcite-tile-href-icon-color-hover token that defaults to the link color

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tile): add design tokens #10476

feat(tile): add design tokens #10476

eriklharper commented Oct 2, 2024 •

edited

Loading

alisonailea left a comment

alisonailea Oct 3, 2024

alisonailea Oct 3, 2024

macandcheese Oct 3, 2024

eriklharper Oct 3, 2024

macandcheese Oct 3, 2024

alisonailea Oct 3, 2024

macandcheese Oct 4, 2024 •

edited

Loading

alisonailea Oct 4, 2024

eriklharper Oct 4, 2024 •

edited

Loading

alisonailea Oct 3, 2024

alisonailea Oct 3, 2024

alisonailea Oct 3, 2024

alisonailea Oct 3, 2024

eriklharper Oct 3, 2024

alisonailea Oct 4, 2024 •

edited

Loading

alisonailea Oct 3, 2024

macandcheese Oct 3, 2024

eriklharper Oct 3, 2024 •

edited

Loading

SkyeSeitz Oct 3, 2024

macandcheese Oct 4, 2024

feat(tile): add design tokens #10476

Are you sure you want to change the base?

feat(tile): add design tokens #10476

Conversation

eriklharper commented Oct 2, 2024 • edited Loading

Summary

alisonailea left a 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 Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriklharper Oct 4, 2024 • edited Loading

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

alisonailea Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriklharper Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriklharper commented Oct 2, 2024 •

edited

Loading

macandcheese Oct 4, 2024 •

edited

Loading

eriklharper Oct 4, 2024 •

edited

Loading

alisonailea Oct 4, 2024 •

edited

Loading

eriklharper Oct 3, 2024 •

edited

Loading