Move component icons to `icons` objects #582

clintcs · 2025-01-06T14:53:07Z

🚀 Description

A wild pattern has emerged! We started using the icons pattern in components where:

An icon is used more than once in a component.
An icon is too large and obscures the markup.

The latter reason is a good one to adopt the pattern wherever we can, I think. Another reason is that it labels icons, which we previously and inconsistently did using code comments.

So I've adopted the pattern throughout. I also deduplicated some icons shared across various components and changed to rem icon widths and heights that were in pixels.

📋 Checklist

I have followed the Contributing Guidelines.
I have added tests to cover new or updated functionality.
I have added or updated Storybook stories.
I have localized new strings.
I have followed the ARIA Authoring Practices Guide or met with the Accessibility Team.
I have included a changeset.
I have scheduled a design review.

🔬 How to Test

Verify that the "x" icon of Input, Inline Alert, Modal, Toast, and Tag matches production.
Verify Accordion, Dropdown, Split Button Secondary Button, Tab Group, and Tree's chevron matches production.
Verify Tree matches production when expanded and when indentation is removed.
Verify Modal's back button and close buttons are announced by VoiceOver.

📸 Images/Videos of Functionality

N/A

changeset-bot · 2025-01-06T14:53:11Z

🦋 Changeset detected

Latest commit: fbe27f8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@crowdstrike/glide-core	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2025-01-06T14:57:20Z

Preview URL

https://glide-core.crowdstrike-ux.workers.dev/move-component-icons-to-icons-object

clintcs · 2025-01-06T14:58:59Z

src/inline-alert.ts

      />
    </svg>
  `,
-  high: svg`


The svg template literal is meant for SVG fragments:

The svg tag function should only be used for SVG fragments, or elements that would be contained inside an HTML element. A common error is placing an element in a template tagged with the svg tag function. The element is an HTML element and should be used within a template tagged with the html tag function.

clintcs · 2025-01-06T16:11:15Z

src/icons/x.ts

+      stroke-linecap="round"
+      stroke-linejoin="round"
+    />
+  </svg>


Used by Input, Inline Alert, Modal, Toast, and Tag.

clintcs · 2025-01-06T16:11:37Z

src/icons/chevron.ts

+      stroke-linecap="round"
+      stroke-linejoin="round"
+    />
+  </svg>


Used by Accordion, Dropdown, Split Button Secondary Button, Tab Group, and Tree.

clintcs · 2025-01-06T16:13:12Z

src/dropdown.ts

                  },
-                  () => {
-                    return html`<svg
-                      aria-label=${this.#localize.term('open')}


Removed aria-label. As far as I can tell, it's not announced by VoiceOver and wouldn't add anything if it were given the presence of aria-expanded. CC: @noahgelmancs.

clintcs · 2025-01-06T16:16:35Z

src/split-button.secondary-button.styles.ts

        cursor: not-allowed;
+
+        svg {
+          color: var(--glide-core-text-tertiary-disabled);


I styled the SVG from .component so I could remove its dynamic .disabled class and move the icon to icons.

Flattening styles is good. But so consistency via an icons object in each component. What do you think? Is the additional nesting worth the consistency?

no strong opinion from me either way

clintcs · 2025-01-06T16:17:21Z

src/tooltip.stories.ts

  render(arguments_) {
+    // A `<span>` is used instead of a `<button>` for VoiceOver, so
+    // "button" isn't read aloud, implying the target can be interacted
+    // with via click.


Aha. Now I remember why it was a <span> instead of <button>.

clintcs · 2025-01-06T16:46:19Z

src/tree.item.test.basics.ts

-
-  expect([
-    ...component.shadowRoot!.querySelector('.expand-icon')!.classList,
-  ]).to.deep.equal(['expand-icon']);


The assertion began failing because I removed this class and styled the SVG from .expand-icon-container. One of my next tasks is to remove tests like these to prepare for VRT. So I removed it instead of fixing it.

clintcs · 2025-01-06T16:48:10Z

src/tree.item.ts

-              ${when(
-                this.hasExpandIcon,
-                () => html`
-                  <div>


It's not obvious what this classless <div> was for—if anything. Any idea @danwenzel?

Nope, seems like you can safely remove it

clintcs · 2025-01-06T16:53:14Z

src/input.stories.ts

    name: '',
    orientation: 'horizontal',
    pattern: '',
+    'password-toggle': false,


clintcs · 2025-01-06T17:13:10Z

src/label.ts

        >
          <span class="optional-tooltip-target" slot="target" tabindex="0">
-            <svg
-              aria-label=${this.#localize.term('moreInformation')}


Removed aria-label. As far as I can tell, it's not announced by VoiceOver and doesn't add anything given the Tooltip is shown immediately on focus. What do you think, @noahgelmancs?

@clintcs Good to go 👍

clintcs · 2025-01-06T17:13:51Z

src/label.ts

 }
+
+const icons = {
+  information: html`


Happy to tweak the name.

clintcs · 2025-01-06T17:14:24Z

src/library/localize.ts


  close: string;
  dismiss: string;
-  open: string;


No longer in use now that it's not used in Dropdown.

clintcs · 2025-01-06T17:14:32Z

src/library/localize.ts

  dismiss: string;
-  open: string;
  selectAll: string;
-  moreInformation: string;


No longer in use now that it's not used in Label.

clintcs · 2025-01-06T17:16:47Z

src/tooltip.stories.ts

      >
        <style>
          [slot="target"] {
-            background-color: transparent;


No longer needed now that the <button> is a <span>.

clintcs · 2025-01-06T17:19:40Z

src/icons/informational.ts

@@ -1,17 +0,0 @@
-import { svg } from 'lit/static-html.js';


Only used by Inline Alert. So I moved it there.

clintcs · 2025-01-06T17:27:41Z

src/label.styles.ts

      }
    }

    .optional-tooltip-target {


"- Form control tooltip focus outlines now hug their "ⓘ" icons."

Before

After

clintcs · 2025-01-06T17:30:57Z

src/tree.item.styles.ts

+        cursor: pointer;
+        display: flex;
+        justify-content: center;
+        rotate: -90deg;


Should we have a single chevron icon and rotate it as needed? Or should we have an icon for each direction?

A feel like a single chevron should be fine, as long as any aria-labels change accordingly

clintcs · 2025-01-06T18:20:41Z

src/tab.group.styles.ts


-        /* "-webkit-scrollbar" is needed for Safari */
-
-        &::-webkit-scrollbar {


Safari supports scrollbar-width as of 18.2.

clintcs · 2025-01-06T18:32:08Z

src/tab.group.styles.ts

      & .overflow {
+        --size: 1.125rem;
+
+        align-items: center;


"- Tab Group's overflow buttons are now vertically centered."

danwenzel · 2025-01-06T19:31:04Z

src/split-button.secondary-button.styles.ts

        cursor: not-allowed;
+
+        svg {
+          color: var(--glide-core-text-tertiary-disabled);


no strong opinion from me either way

danwenzel · 2025-01-06T19:33:00Z

src/tree.item.styles.ts

+        cursor: pointer;
+        display: flex;
+        justify-content: center;
+        rotate: -90deg;


A feel like a single chevron should be fine, as long as any aria-labels change accordingly

ynotdraw · 2025-01-07T16:08:13Z

src/tree.item.styles.ts

+        cursor: pointer;
+        display: flex;
+        justify-content: center;
+        rotate: -90deg;


clintcs · 2025-01-09T18:28:26Z

Pushed an amended commit resolving the conflicts.