Nimble chip #2689

atmgrifter00 · 2025-08-11T20:29:37Z

Pull Request

🤨 Rationale

Creating the nimble-chip component for the immediate requirement of adding attachments to the spright-chat-input component.

👩‍💻 Implementation

Typical Nimble implementation. Adding a new label provider for the 'Remove' text for the remove button.

🧪 Testing

Unit and chromatic tests.

✅ Checklist

I have updated the project documentation to reflect my changes or determined no changes are needed.

jattasNI

Mostly reviewed docs / behavior / style, not much of the implementation.

packages/storybook/src/nimble/chip/chip.stories.ts

change/@ni-nimble-components-fa1ae624-6b7b-4035-b6bd-1418d8451b11.json

packages/storybook/src/nimble/chip/chip.mdx

packages/nimble-components/src/chip/tests/chip.spec.ts

packages/nimble-components/src/chip/styles.ts

packages/nimble-components/src/chip/template.ts

packages/nimble-components/src/chip/styles.ts

packages/nimble-components/src/label-provider/core/index.ts

…e-chip

packages/nimble-components/src/chip/styles.ts

packages/nimble-components/src/chip/index.ts

packages/nimble-components/src/chip/template.ts

packages/nimble-components/src/chip/styles.ts

mollykreis · 2025-08-18T16:30:37Z

packages/nimble-components/src/chip/styles.ts

+    }
+
+    .remove-button {
+        height: 16px;


Does the design really call for a 16x16 button? That seems too small for accessibility and mobile reasons.

It does. I'll let @NIbokeefe offer his reasoning here, but for my part, I do like the look of this over the other standard options (ignoring the accessibility concerns). I'll note that there are at least other design systems that have a similar small hit area for the remove button on chips.

@mollykreis, For mobile accessibility it requires a 48x48 touchpoint which is larger than our largest controls. We won't technically be accessible for mobile without restructuring our sizing ramp. Often we use 24px buttons in 32px controls, and 18px buttons in 24px controls. That being said, those dimensions only pertaining to the controls visual representation, that doesn't mean the "hotspot" touch area can't or shouldn't be larger. In most cases increasing the size of the hotspot area would improve the interaction, so long as it doesn't interfere with the bounds of another touchpoint.

In a chip, I'm calling for a 16x16 button (which is a standard sized button) but the hotspot-clickable region can be most of the right surrounding area. This sized button counter balances and maintain symmetry with the icon.

We don't currently have a way for the touchpoint to be larger than the button, so we'd need to be fine with 16x16 touch point for now.

Also, we never added conditional styling to the button to change the focus-visible styling of the button when it got smaller (shouldn't have the double focus rings when small). Are we fine having the double ring on the 16x16 button until if/when that button styling gets updated?

@mollykreis In my spec I drew it up with one 2px ring just like a Rollover state. Due to its small size and likelihood of covering the icon, I skipped the inner ring. But, I don't mind your mockup with the two rings, I could go either way.

…e-chip

packages/storybook/src/nimble/chip/chip.stories.ts

packages/storybook/src/nimble/chip/chip.mdx

packages/storybook/src/nimble/chip/chip-matrix.stories.ts

packages/storybook/src/docs/component-status.stories.ts

packages/nimble-components/src/chip/styles.ts

packages/nimble-components/src/chip/testing/chip.pageobject.ts

packages/nimble-components/src/chip/tests/chip.spec.ts

Co-authored-by: Jesse Attas <jattasNI@users.noreply.github.com>

…e-chip

packages/nimble-components/src/chip/styles.ts

mollykreis · 2025-08-20T14:46:02Z

packages/nimble-components/src/chip/styles.ts

+        ChipAppearance.block,
+        css`
+            :host {
+                background-image: linear-gradient(


Why did you need to set a background-image to achieve a solid background color?

This is what the button does. From what I can this is done to remove the border?

With background-image:

Using background-color = :

Design:

I'll try a different route, however, and just have a transparent border, so we don't see that fuzziness at the corners.

It looks like there is still some background-image related styling in the component. Can you take a pass at making sure it's all been converted to background colors?

mollykreis · 2025-08-20T14:48:06Z

packages/nimble-components/src/chip/tests/chip.spec.ts

+    return await fixture<Chip>(html`<${chipTag}></${chipTag}>`);
+}
+
+describe('Chip', () => {


Can you add some tests for the tab index behavior?

mollykreis · 2025-08-20T14:48:09Z

packages/nimble-components/src/chip/template.ts

+            <${buttonTag}
+                class="remove-button"
+                content-hidden
+                appearance="ghost"


Can you use ButtonAppearance.ghost here?

mollykreis · 2025-08-20T14:52:25Z

packages/storybook/src/nimble/chip/chip-matrix.stories.ts

+
+const widthStates = [
+    ['Default Width', ''],
+    ['Custom Width', 'width: 250px;']


If you haven't already, can you confirm with @NIbokeefe that when the chip is wider than the default "fit-text" width, that the remove button should stick to the text rather than the edge of the chip?

FWIW - in my opinion, the remove button should always be right-aligned in the chip rather than floating in the middle next text. I also wonder if the text & icon should always be left aligned rather than centered.

@NIbokeefe, can you provide guidance here. While this may be an atypical scenario, it's certainly something a user could easily do, so we should style this as expected.

@atmgrifter00 @mollykreis,

I don't see the use case for wanting to expand a chip's width. We do allow that with buttons. However, with chips anywhere in the short or long term they should be nested within a control and not a stand alone selection element that would require modified margins to fill spaces.

If it makes sense to build that functionality into the control regardless, I agree with @mollykreis, the text and icon should left align, and the close button should right align as shown in example "A." We should prevent this from happening until we have a use case that warrants it.

mollykreis · 2025-08-20T14:54:26Z

packages/storybook/src/nimble/chip/chip-matrix.stories.ts

+        active: []
+    })
+);
+export const textCustomized: StoryFn = createMatrixThemeStory(


The italic text looks clipped in the Text Customized story.

Yeah, it is, and I talked with @jattasNI about this, but it's unclear to me what's causing it. The button doesn't exhibit this behavior, and even if I align the styles for the chip and the button (for the content part), I can't get rid of the cut-off. Interested in suggestions here.

Some posts on stackoverflow seem to show this as a known issue that italics break out of their computed size on layout. This post says that synthetic italics seem to do it more. We ship bold fonts to avoid synthetic bold / cross browser issues so if we were inclined to fix this I think we should try shipping actual italic fonts. I don't think I'm inclined to try the other workarounds in those posts (extra fake padding, changing letter spacing, etc).

Also thought maybe the new CSS text-box-trim and text-box-edge properties may help. They actually may fix alignment issues we have had with buttons, see the centering and alignment sections: https://developer.chrome.com/blog/css-text-box-trim#what_can_i_build_with_it_or_what_problems_does_it_solve
But playing around they seem to truncate the text-box even more while in this case we want to grow it :P

Inclined to treat it as a known issue for now unless there is a reasonable non-hacky fix

mollykreis · 2025-08-20T14:56:15Z

packages/storybook/src/nimble/chip/chip.mdx

+- chips are arranged horizontally and wrap to new lines when there is
+  insufficient horizontal space
+- all chips have the same appearance
+- use the medium-padding token to create space between chips


Assuming medium padding token is intended to be used for the horizontal and vertical spacing between chips, can you explicitly state that to avoid confusion?

…e-chip

rajsite · 2025-08-20T18:47:20Z

packages/nimble-components/src/chip/template.ts

+ViewTemplate<Chip>,
+ChipOptions
+> = (context, definition) => html<Chip>`
+    <template>


Don't need the outer wrapping template if it has no bindings on it (represents binding to the host itself, i.e. nimble-chip)

rajsite · 2025-08-20T19:04:27Z

packages/nimble-components/src/chip/template.ts

+                ${x => x.removeButtonContent}
+            </${buttonTag}>
+        `)}
+    </template>


Can you add the endSlotTemplate as well since the mixin includes both. It should just be styled dispaly:none;

rajsite · 2025-08-20T19:10:04Z

packages/nimble-components/src/label-provider/core/index.ts

    scrollBackward: scrollBackwardLabel,
-    scrollForward: scrollForwardLabel
+    scrollForward: scrollForwardLabel,
+    chipRemove: chipRemoveLabel


I think we can just go with remove as the name. I think that label will be generic enough for multiple contexts and we don't need an explicit remove label just for chip. If done make sure to update corresponding attributes, etc

rajsite · 2025-08-20T19:12:39Z

packages/nimble-components/src/chip/index.ts

+
+    /** @internal */
+    public get removeButtonContent(): string {
+        return `${chipRemoveLabel.getValueFor(this)} ${this.elementTextContent}`;


Will the <Remove> <Label Text> string concatenation apply across languages?
Wondering if we should just keep it as "Remove" for now unless we have confidence in the l10n and aria design for chip/pill/badge concepts

rajsite · 2025-08-20T19:30:35Z

packages/nimble-components/src/chip/styles.ts

+        cursor: default;
+        color: ${bodyDisabledFontColor};
+        border-color: rgba(${borderRgbPartialColor}, 0.3);
+        box-shadow: none;


looks like box-shadow is always none? do we need this style?

@rajsite I can't currently think of a reason we would use a box shadow (elevation) on a chip to date. Maybe a future iteration we use elevate a control with a box-shadow on rollover, but this would become a universal rule not just for chips alone.

rajsite · 2025-08-20T19:31:09Z

packages/nimble-components/src/chip/styles.ts

+    :host([disabled])::before {
+        box-shadow: none;
+    }


looks like box-shadow is always none? do we need this style?

rajsite · 2025-10-03T20:43:02Z

change/@ni-nimble-components-fa1ae624-6b7b-4035-b6bd-1418d8451b11.json

@@ -0,0 +1,7 @@
+{


@atmgrifter00 still intend to get this pushed through?

I can try and find some downtime to resolve the remaining issues, and get it pushed in.

atmgrifter00 and others added 14 commits July 25, 2025 18:05

Nimble chip with tests.

bae0a03

Fix tab focus. Add label provider string for remove.

a716932

Prettier.

59e3f91

Minor fixes.

aef1b17

Clean up.

0414761

Language updates.

2325b54

Merge branch 'main' into nimble-chip

757a50b

Linting.

a2fdd04

Fix tests.

ce0889c

Change style to default to outline appearance.

c864b5d

Prettier.

9c318f4

Change files

4c927df

Merge branch 'main' into nimble-chip

90dfa96

Merge branch 'main' into nimble-chip

00f66c5

atmgrifter00 marked this pull request as ready for review August 14, 2025 17:45

atmgrifter00 requested review from jattasNI and rajsite as code owners August 14, 2025 17:45

atmgrifter00 marked this pull request as draft August 14, 2025 19:39

jattasNI reviewed Aug 14, 2025

View reviewed changes

atmgrifter00 added 9 commits August 14, 2025 17:14

Remove unneeded tests. Safari focus fix.

1fbc342

Merge branch 'nimble-chip' of https://github.com/ni/nimble into nimbl…

3461d3c

…e-chip

Fix disabled state of remove button

b5d2016

Remove unecessary delete and backspace key handling.

6cffc95

Handle PR feedback.

70d61ad

Merge from main.

c13e138

Handle PR feedback.

5b80615

Prettier.

fa47ea7

Added test.

1561b41

atmgrifter00 commented Aug 18, 2025

View reviewed changes

packages/nimble-components/src/chip/styles.ts Outdated Show resolved Hide resolved

Fix storybook Chip remove event listener.

dd5643a

atmgrifter00 marked this pull request as ready for review August 18, 2025 15:26

atmgrifter00 requested a review from mollykreis August 18, 2025 15:26

mollykreis reviewed Aug 18, 2025

View reviewed changes

atmgrifter00 added 3 commits August 18, 2025 13:26

Handle PR feedback.

271b05b

Merge branch 'nimble-chip' of https://github.com/ni/nimble into nimbl…

9441764

…e-chip

Fix style.

1a8bafc

atmgrifter00 requested review from NIbokeefe, jattasNI and mollykreis August 18, 2025 21:31

jattasNI approved these changes Aug 18, 2025

View reviewed changes

atmgrifter00 and others added 8 commits August 19, 2025 08:07

Update packages/nimble-components/src/chip/tests/chip.spec.ts

42737f5

Co-authored-by: Jesse Attas <jattasNI@users.noreply.github.com>

Update packages/nimble-components/src/chip/tests/chip.spec.ts

e0615c1

Co-authored-by: Jesse Attas <jattasNI@users.noreply.github.com>

Merge branch 'main' into nimble-chip

473bce2

Handle PR feedback.

b45c0b0

Merge branch 'nimble-chip' of https://github.com/ni/nimble into nimbl…

220d3f8

…e-chip

Test cleanup.

6c35546

Revert debug changes.

311377c

Use token for remove button size.

a638378

mollykreis reviewed Aug 20, 2025

View reviewed changes

atmgrifter00 added 3 commits August 20, 2025 11:52

Fix block styling.

6acf509

Merge branch 'nimble-chip' of https://github.com/ni/nimble into nimbl…

128fbcb

…e-chip

Handle PR feedback.

31f79ae

rajsite approved these changes Aug 20, 2025

View reviewed changes

rajsite reviewed Oct 3, 2025

View reviewed changes

Nimble chip #2689

Are you sure you want to change the base?

Nimble chip #2689

Uh oh!

Conversation

atmgrifter00 commented Aug 11, 2025

Pull Request

🤨 Rationale

👩‍💻 Implementation

🧪 Testing

✅ Checklist

Uh oh!

jattasNI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NIbokeefe Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atmgrifter00 Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NIbokeefe Aug 25, 2025 •

edited

Loading

atmgrifter00 Aug 20, 2025 •

edited

Loading

rajsite Aug 20, 2025 •

edited

Loading