feat(chip): add component tokens #10179

alisonailea · 2024-08-28T20:27:50Z

Related Issue: #7180

Summary

Adds tokens, e2e, and custom-theme chromatic tests. This ended up being a larger refactor to clarify internal tokens.

Chip Tokens

--calcite-chip-background-color: Specifies the background color of the component.
--calcite-chip-border-color: Specifies the border color of the component.
--calcite-chip-corner-radius: Specifies the corner radius of the component.
--calcite-chip-text-color: Specifies the text color of the component.
--calcite-chip-icon-color: Specifies the icon color of the component.
--calcite-chip-close-icon-color: Specifies the icon color of the close element of the component.
--calcite-chip-select-icon-color: Specifies the icon color of the selection element of the component.
--calcite-chip-select-icon-color-pressed: Specifies the icon color of the selection element of the component when active.

Chip Group Tokens

--calcite-chip-group-items-space: Specifies spacing between items in the component.

macandcheese · 2024-08-28T20:35:37Z

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

+ * @prop --calcite-chip-border-color: Specifies the border color of the component.
+ * @prop --calcite-chip-corner-radius: Specifies the border radius of the component.
+ * @prop --calcite-chip-text-color: Specifies the text color of the component.
+ *


I think we'd at the very least want to add --calcite-chip-icon-color and --calcite-chip-selection-icon-color (or whatever name) from the 7180 PR:

calcite-design-system/packages/calcite-components/src/components/chip/chip.scss

Line 10 in 2c5e985

* @prop --calcite-chip-icon-color: Specifies the color of the component's icon property.

We can't just set --calcite-icon-color on a Calcite Chip - it has too many internally rendered icons that would be affected. The 7180 PR also had --calcite-chip-close-icon-color (and, props to control internal close button states, which it sounds like we did not want to pursue - I still think this prevents a lot of valid use cases)

The design pattern for calcite-chip is for icons to follow the text color. If a user wants to have a special color they can do something like calcite-chip.my-chip { --calicte-icon-color: red }. Similarly. IMO, all close sub-elements (should) have the same interaction pattern with a shared token set. Something like

--calcite-close-color --calcite-close-background-color-hover --calcite-close-background-color-press

@SkyeSeitz @ashetland

Agreed they should be (ideally) consistently implemented when overridden - but not adding component-specific ones (even if it falls back to the global one) to the component means they won't be documented alongside all the other component tokens - wherever that doc is consumed (including places we can't manually supplement it with info about these generic "close-color" props).

Currently it seems like text-color property here also sets the selection affordance icon as well as the icon-start icon. At the very least I'd like to be able to not have this happen (which is the current behavior):

The current behavior that is available (just overriding the global css prop - changes the affordance as well as the icon, which I don't think is always preferable).

My use case for overriding an icon color is probably to match map symbology or layer display color / graphics (or something like that). So I don't think I'd want the selection icon to change as well:

macandcheese · 2024-08-28T20:37:02Z

Do we want to kick off Chromatic builds for these token PR that include refactors?

…openPR/7180-chip # Conflicts: # packages/calcite-components/src/components/chip/chip.scss

alisonailea · 2024-08-28T20:43:25Z

Do we want to kick off Chromatic builds for these token PR that include refactors?

Yes all token PRs need to pass Chromatic tests because we need to ensure they are not creating unexpected visual changes. I've done manual checks but human error is real.

macandcheese

I think the spacing props need a pass here for certain configurations:

This reverts commit b052c8d.

After an inspection of the chip component in figma this adds additional internal variables to clarify what elements are changing and based on what state changes. Also breaks out reusable .close mixin

resolves some issues with internal component spacing. It also normalizes internal variables and makes future maintenance easier by breaking variables out by sub-element.

macandcheese

Looks good! Just some comments on token naming, and also confirming that:

with this set up, users can't independently control the selection affordance icon - just the "icon rendered via prop" and the close button.

Could you kick off another Chromatic build to test latest changes? Thanks!

macandcheese · 2024-09-10T20:51:11Z

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

+ *
+ * @prop --calcite-chip-background-color: Specifies the background color of the component.
+ * @prop --calcite-chip-border-color: Specifies the border color of the component.
+ * @prop --calcite-chip-corner-radius: Specifies the border radius of the component.


For the description here do we want to also use "corner radius"?

macandcheese · 2024-09-10T20:53:20Z

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

+ * @prop --calcite-chip-corner-radius: Specifies the border radius of the component.
+ * @prop --calcite-chip-text-color: Specifies the text color of the component.
+ * @prop --calcite-chip-icon-color: Specifies the icon color of the component.
+ * @prop --calcite-close-icon-color: Specifies the icon color of the close element of the component.


I realize this is "global" but it still (IMO) would be nice to scope this to the component. That way, both --calcite-close-icon-color works (at the global level) and --calcite-chip-close-icon-color (at the component level) could work, and we don't need to document non-component-scoped tokens inside a component.

Confirming we are not allowing the Chip's selection affordance to change independently of the "icon rendered from prop".

jcfranco

Aside from a few comments, this looks great!

jcfranco · 2024-09-10T23:33:54Z

packages/calcite-components/src/components/chip/chip.e2e.ts

+    });
+
+    describe("appearance='outline'", () => {
+      themed(html`<calcite-chip appearance="outline" kind="neutral">Layers</calcite-chip>`, {


Is kind=neutral important for this test? If so, can we rename the describe block to convey this? Otherwise, let's remove it.

jcfranco · 2024-09-10T23:51:10Z

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

-      --calcite-internal-chip-padding-end: var(--calcite-spacing-base);
+:host([scale="s"]) {
+  .container {
+    --calcite-internal-chip-block-size: 1.5rem /* 24px */;


Instead of adding comments with the equivalent pixel size for all of these, may I suggest adding this to our extensions? It should help reduce noise, overhead, and the chance of them being overlooked when updated.

If that looks like a good path forward, we can update the extensions in a separate PR.

I was doing this because these need to be replaced with the upcoming spacing tokens which will all use rems instead of px

jcfranco · 2024-09-11T00:04:31Z

packages/calcite-components/src/assets/styles/includes.scss

@@ -152,3 +152,40 @@
    }
  }
 }
+
+@mixin close-button($size: "var(--calcite-internal-close-size, 1.5rem /* 24px */)", $padding: "0") {


Sidebar: we'll need to consolidate this with the x-button mixin above. Related #6640.

jcfranco · 2024-09-11T00:07:53Z

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

  box-border
  font-medium;

+  border-radius: var(--calcite-chip-corner-radius, 9999px);


Sidebar: we still need a semantic token for this fallback, right?

yes this will be coming in future updates

jcfranco · 2024-09-11T00:12:43Z

@alisonailea Before merging, can you update the PR description with the latest component tokens?

jcfranco · 2024-09-11T20:00:26Z

package-lock.json

@@ -3727,15 +3727,16 @@
      }
    },
    "node_modules/@inquirer/core": {


Can you roll back the this change?

This reverts commit b6e6c97.

…openPR/7180-chip # Conflicts: # packages/calcite-components/src/custom-theme.stories.ts

* origin/dev: feat(chip): add component tokens (#10179) feat(avatar): add component tokens (#10280) fix(popover): update focus trap elements on mutation observer (#10357) fix(action-pad): update layout on action-group elements slotted after initialization (#10355)

alisonailea added 5 commits August 27, 2024 15:48

feat(chip): refactor chip for component tokens

9507e9a

test(chip): add component tokens to stories

c6d5f1b

fix(chip): remove mouse state tokens

a7ddcc2

test(chip): update e2e tests

b226cb9

feat(chip-group): add component tokens

b052c8d

alisonailea self-assigned this Aug 28, 2024

alisonailea requested review from jcfranco, macandcheese and Elijbet August 28, 2024 20:27

macandcheese reviewed Aug 28, 2024

View reviewed changes

Merge branch 'dev' of github.com:Esri/calcite-design-system into ali-…

865757a

…openPR/7180-chip # Conflicts: # packages/calcite-components/src/components/chip/chip.scss

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

alisonailea mentioned this pull request Aug 28, 2024

Design token phase II - Integration across components #7180

Open

macandcheese reviewed Aug 28, 2024

View reviewed changes

alisonailea added 3 commits August 30, 2024 11:02

Revert "feat(chip-group): add component tokens"

3b0ac61

This reverts commit b052c8d.

fix(chip): chip element spacing

2332a09

After an inspection of the chip component in figma this adds additional internal variables to clarify what elements are changing and based on what state changes. Also breaks out reusable .close mixin

fix(chip): internal variables in selection states

5fe9b0c

alisonailea added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Sep 3, 2024

alisonailea added 2 commits September 9, 2024 17:00

fix(chip): minor issues with spacing

819021c

resolves some issues with internal component spacing. It also normalizes internal variables and makes future maintenance easier by breaking variables out by sub-element.

fix(chip): add chip-icon-color token

798c7c4

alisonailea requested a review from macandcheese September 10, 2024 19:47

macandcheese approved these changes Sep 10, 2024

View reviewed changes

fix(chip): add select and close token options

f9d0ec0

alisonailea removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Sep 10, 2024

jcfranco approved these changes Sep 11, 2024

View reviewed changes

test(chip): simplify theme tests

b6e6c97

alisonailea requested a review from benelan as a code owner September 11, 2024 18:19

test(chip): fix theme setup

d9cad9a

jcfranco reviewed Sep 11, 2024

View reviewed changes

Revert package.json in test(chip): simplify theme tests”

be4ac0f

This reverts commit b6e6c97.

alisonailea added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Sep 12, 2024

alisonailea added 2 commits September 11, 2024 18:21

refactor(chip): restore —calcite-icon-color styles

a709177

Merge branch 'dev' of github.com:Esri/calcite-design-system into ali-…

89fa877

…openPR/7180-chip # Conflicts: # packages/calcite-components/src/custom-theme.stories.ts

jcfranco mentioned this pull request Sep 13, 2024

chore: add cssrem VSCode extension recommendation #10300

Draft

alisonailea added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 13, 2024

Merge branch 'dev' into astump/7180-chip

a8e74a8

alisonailea removed the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Sep 13, 2024

Merge branch 'dev' into astump/7180-chip

9ce58e7

alisonailea added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Sep 16, 2024

chore(custom-theme): fix imports

fa3f3e7

alisonailea added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Sep 19, 2024

alisonailea merged commit ff82570 into dev Sep 20, 2024
13 checks passed

alisonailea deleted the astump/7180-chip branch September 20, 2024 17:27

github-actions bot added this to the 2024-09-24 - 2.13.0 Sep Main release milestone Sep 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(chip): add component tokens #10179

feat(chip): add component tokens #10179

alisonailea commented Aug 28, 2024 •

edited

Loading

macandcheese Aug 28, 2024 •

edited

Loading

alisonailea Aug 28, 2024 •

edited

Loading

macandcheese Aug 28, 2024 •

edited

Loading

macandcheese commented Aug 28, 2024

alisonailea commented Aug 28, 2024

macandcheese left a comment

macandcheese left a comment

macandcheese Sep 10, 2024

macandcheese Sep 10, 2024

jcfranco left a comment

jcfranco Sep 10, 2024

jcfranco Sep 10, 2024

alisonailea Sep 11, 2024

jcfranco Sep 11, 2024

jcfranco Sep 11, 2024

alisonailea Sep 11, 2024

jcfranco commented Sep 11, 2024

jcfranco Sep 11, 2024

feat(chip): add component tokens #10179

feat(chip): add component tokens #10179

Conversation

alisonailea commented Aug 28, 2024 • edited Loading

Summary

Chip Tokens

Chip Group Tokens

macandcheese Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

alisonailea Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

macandcheese Aug 28, 2024 • edited Loading

Choose a reason for hiding this comment

macandcheese commented Aug 28, 2024

alisonailea commented Aug 28, 2024

macandcheese left a comment

Choose a reason for hiding this comment

macandcheese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfranco 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

jcfranco commented Sep 11, 2024

Choose a reason for hiding this comment

alisonailea commented Aug 28, 2024 •

edited

Loading

macandcheese Aug 28, 2024 •

edited

Loading

alisonailea Aug 28, 2024 •

edited

Loading

macandcheese Aug 28, 2024 •

edited

Loading