Skip to content
Open
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
bae0a03
Nimble chip with tests.
atmgrifter00 Jul 25, 2025
a716932
Fix tab focus. Add label provider string for remove.
atmgrifter00 Jul 28, 2025
59e3f91
Prettier.
atmgrifter00 Jul 28, 2025
aef1b17
Minor fixes.
atmgrifter00 Aug 11, 2025
0414761
Clean up.
atmgrifter00 Aug 11, 2025
2325b54
Language updates.
atmgrifter00 Aug 11, 2025
757a50b
Merge branch 'main' into nimble-chip
atmgrifter00 Aug 11, 2025
a2fdd04
Linting.
atmgrifter00 Aug 12, 2025
ce0889c
Fix tests.
atmgrifter00 Aug 12, 2025
c864b5d
Change style to default to outline appearance.
atmgrifter00 Aug 12, 2025
9c318f4
Prettier.
atmgrifter00 Aug 12, 2025
4c927df
Change files
atmgrifter00 Aug 12, 2025
90dfa96
Merge branch 'main' into nimble-chip
atmgrifter00 Aug 13, 2025
00f66c5
Merge branch 'main' into nimble-chip
atmgrifter00 Aug 14, 2025
1fbc342
Remove unneeded tests. Safari focus fix.
atmgrifter00 Aug 14, 2025
3461d3c
Merge branch 'nimble-chip' of https://github.com/ni/nimble into nimbl…
atmgrifter00 Aug 14, 2025
b5d2016
Fix disabled state of remove button
atmgrifter00 Aug 14, 2025
6cffc95
Remove unecessary delete and backspace key handling.
atmgrifter00 Aug 14, 2025
70d61ad
Handle PR feedback.
atmgrifter00 Aug 17, 2025
c13e138
Merge from main.
atmgrifter00 Aug 18, 2025
5b80615
Handle PR feedback.
atmgrifter00 Aug 18, 2025
fa47ea7
Prettier.
atmgrifter00 Aug 18, 2025
1561b41
Added test.
atmgrifter00 Aug 18, 2025
dd5643a
Fix storybook Chip remove event listener.
atmgrifter00 Aug 18, 2025
271b05b
Handle PR feedback.
atmgrifter00 Aug 18, 2025
9441764
Merge branch 'nimble-chip' of https://github.com/ni/nimble into nimbl…
atmgrifter00 Aug 18, 2025
1a8bafc
Fix style.
atmgrifter00 Aug 18, 2025
42737f5
Update packages/nimble-components/src/chip/tests/chip.spec.ts
atmgrifter00 Aug 19, 2025
e0615c1
Update packages/nimble-components/src/chip/tests/chip.spec.ts
atmgrifter00 Aug 19, 2025
473bce2
Merge branch 'main' into nimble-chip
atmgrifter00 Aug 19, 2025
b45c0b0
Handle PR feedback.
atmgrifter00 Aug 19, 2025
220d3f8
Merge branch 'nimble-chip' of https://github.com/ni/nimble into nimbl…
atmgrifter00 Aug 19, 2025
6c35546
Test cleanup.
atmgrifter00 Aug 19, 2025
311377c
Revert debug changes.
atmgrifter00 Aug 19, 2025
a638378
Use token for remove button size.
atmgrifter00 Aug 19, 2025
6acf509
Fix block styling.
atmgrifter00 Aug 20, 2025
128fbcb
Merge branch 'nimble-chip' of https://github.com/ni/nimble into nimbl…
atmgrifter00 Aug 20, 2025
31f79ae
Handle PR feedback.
atmgrifter00 Aug 20, 2025
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

@atmgrifter00 still intend to get this pushed through?

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 can try and find some downtime to resolve the remaining issues, and get it pushed in.

"type": "minor",
"comment": "Adding Nimble Chip component",
"packageName": "@ni/nimble-components",
"email": "26874831+atmgrifter00@users.noreply.github.com",
"dependentChangeType": "patch"
}
72 changes: 72 additions & 0 deletions packages/nimble-components/src/chip/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { attr, observable } from '@ni/fast-element';
import {
applyMixins,
DesignSystem,
FoundationElement,
StartEnd,
type FoundationElementDefinition,
type StartOptions
} from '@ni/fast-foundation';
import { styles } from './styles';
import { template } from './template';
import { ChipAppearance } from './types';
import { slotTextContent } from '../utilities/models/slot-text-content';
import { chipRemoveLabel } from '../label-provider/core/label-tokens';

declare global {
interface HTMLElementTagNameMap {
'nimble-chip': Chip;
}
}

export type ChipOptions = FoundationElementDefinition & StartOptions;

/**
* A Nimble demo component (not for production use)
*/
export class Chip extends FoundationElement {
@attr({ mode: 'boolean' })
public removable = false;

@attr({ mode: 'boolean' })
public disabled = false;

@attr()
public appearance: ChipAppearance = 'outline';

/** @internal */
public readonly content?: HTMLElement[];

/** @internal */
@observable
public hasOverflow = false;

/** @internal */
public get elementTextContent(): string {
return slotTextContent(this.contentSlot);
}

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

@rajsite rajsite Aug 20, 2025

Choose a reason for hiding this comment

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

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

}

/** @internal */
public contentSlot!: HTMLSlotElement;

public handleRemoveClick(): void {
if (this.removable) {
this.$emit('remove');
}
}
}
applyMixins(Chip, StartEnd);

const nimbleChip = Chip.compose<ChipOptions>({
baseName: 'chip',
template,
styles
});

DesignSystem.getOrCreate().withPrefix('nimble').register(nimbleChip());
export const chipTag = 'nimble-chip';
112 changes: 112 additions & 0 deletions packages/nimble-components/src/chip/styles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
import { css } from '@ni/fast-element';
import {
actionRgbPartialColor,
bodyDisabledFontColor,
bodyFont,
bodyFontColor,
borderRgbPartialColor,
borderWidth,
controlHeight,
iconColor,
mediumPadding,
smallPadding
} from '../theme-provider/design-tokens';
import { display } from '../utilities/style/display';
import { appearanceBehavior } from '../utilities/style/appearance';
import { focusVisible } from '../utilities/style/focus';
import { ChipAppearance } from './types';

export const styles = css`
${display('inline-flex')}
:host {
height: ${controlHeight};
width: fit-content;
max-width: 300px;
color: ${bodyFontColor};
font: ${bodyFont};
padding: 0 ${mediumPadding};
gap: 4px;
background-color: transparent;
border: ${borderWidth} solid rgba(${actionRgbPartialColor}, 0.3);
border-radius: 4px;
justify-content: center;
align-items: center;
background-repeat: no-repeat;
${
/*
Not sure why but this is needed to get buttons with icons and buttons
without icons to align on the same line when the button is inline-flex
See: https://github.com/ni/nimble/issues/503
*/ ''
}
vertical-align: middle;
}
:host([disabled]) {
cursor: default;
color: ${bodyDisabledFontColor};
border-color: rgba(${borderRgbPartialColor}, 0.3);
box-shadow: none;
Copy link
Member

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

@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.

background-image: none;
background-size: 100% 100%;
}
:host([disabled])::before {
box-shadow: none;
}
Comment on lines +47 to +49
Copy link
Member

Choose a reason for hiding this comment

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

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

:host([disabled]) slot[name='start']::slotted(*),
:host([disabled]) slot[name='end']::slotted(*),
:host([disabled]) .remove-button {
opacity: 0.3;
${iconColor.cssCustomProperty}: ${bodyFontColor};
}
slot[name='start']::slotted(*) {
flex-shrink: 0;
}
[part='start'],
[part='end'] {
display: contents;
${iconColor.cssCustomProperty}: ${bodyFontColor};
}
.content {
text-overflow: ellipsis;
overflow: hidden;
white-space: nowrap;
}
.remove-button {
height: 16px;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

@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.

Screenshot 2025-08-18 at 11 34 53 AM

Copy link
Contributor

Choose a reason for hiding this comment

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

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?
image

Copy link

@NIbokeefe NIbokeefe Aug 25, 2025

Choose a reason for hiding this comment

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

@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.

Screenshot 2025-08-25 at 1 32 43 PM

width: 16px;
margin-right: calc(-1 * ${smallPadding});
}
`.withBehaviors(
appearanceBehavior(
ChipAppearance.block,
css`
:host {
background-image: linear-gradient(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you need to set a background-image to achieve a solid 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.

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

With background-image:
image

Using background-color = :
image

Design:
image

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

rgba(${borderRgbPartialColor}, 0.1),
rgba(${borderRgbPartialColor}, 0.1)
);
border-color: rgba(${borderRgbPartialColor}, 0.1);
}
:host(${focusVisible}) {
background-size: calc(100% - 4px) calc(100% - 4px);
}
:host([disabled]) {
background-image: linear-gradient(
rgba(${borderRgbPartialColor}, 0.1),
rgba(${borderRgbPartialColor}, 0.1)
);
border-color: rgba(${borderRgbPartialColor}, 0.1);
}
`
)
);
45 changes: 45 additions & 0 deletions packages/nimble-components/src/chip/template.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { html, ref, slotted, when, type ViewTemplate } from '@ni/fast-element';
import {
startSlotTemplate,
type FoundationElementTemplate
} from '@ni/fast-foundation';
import type { Chip, ChipOptions } from '.';
import { overflow } from '../utilities/directive/overflow';
import { buttonTag } from '../button';
import { iconTimesTag } from '../icons/times';

// prettier-ignore
export const template: FoundationElementTemplate<
ViewTemplate<Chip>,
ChipOptions
> = (context, definition) => html<Chip>`
<template>
Copy link
Member

Choose a reason for hiding this comment

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

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

${startSlotTemplate(context, definition)}
<span
class="content"
part="content"
${overflow('hasOverflow')}
title=${x => (x.hasOverflow && x.elementTextContent
? x.elementTextContent
: null)}
>
<slot
${ref('contentSlot')}
${slotted({ property: 'content' })}
></slot>
</span>
${when(x => x.removable && !x.disabled, html<Chip>`
<${buttonTag}
class="remove-button"
content-hidden
appearance="ghost"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use ButtonAppearance.ghost here?

?disabled="${x => x.disabled}"
tabindex="${x => (!x.disabled ? '0' : null)}"
@click="${x => x.handleRemoveClick()}"
>
<${iconTimesTag} slot="start"></${iconTimesTag}>
${x => x.removeButtonContent}
</${buttonTag}>
`)}
</template>
Copy link
Member

Choose a reason for hiding this comment

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

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

`;
32 changes: 32 additions & 0 deletions packages/nimble-components/src/chip/testing/chip.pageobject.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import type { Chip } from '..';
import type { Button } from '../../button';

/**
* Page object for the `nimble-chip` component to provide consistent ways
* of querying and interacting with the component during tests.
*/
export class ChipPageObject {
public constructor(protected readonly chipElement: Chip) {}

public isRemoveButtonVisible(): boolean {
const removeButton = this.getRemoveButton();
return removeButton ? removeButton.offsetParent !== null : false;
}

public clickRemoveButton(): void {
const removeButton = this.getRemoveButton();
if (removeButton) {
removeButton.click();
} else {
throw new Error('Remove button not found');
}
}

private getRemoveButton(): Button | null {
return (
this.chipElement.shadowRoot?.querySelector<Button>(
'.remove-button'
) ?? null
);
}
}
81 changes: 81 additions & 0 deletions packages/nimble-components/src/chip/tests/chip.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import { html } from '@ni/fast-element';
import { Chip, chipTag } from '..';
import { fixture, type Fixture } from '../../utilities/tests/fixture';
import { ChipPageObject } from '../testing/chip.pageobject';
import { waitForUpdatesAsync } from '../../testing/async-helpers';

async function setup(): Promise<Fixture<Chip>> {
return await fixture<Chip>(html`<${chipTag}></${chipTag}>`);
}

describe('Chip', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some tests for the tab index behavior?

let element: Chip;
let connect: () => Promise<void>;
let disconnect: () => Promise<void>;

beforeEach(async () => {
({ element, connect, disconnect } = await setup());
await connect();
});

afterEach(async () => {
await disconnect();
});

it('can construct an element instance', () => {
expect(document.createElement(chipTag)).toBeInstanceOf(Chip);
});

it('click remove button raises remove event', async () => {
element.removable = true;
await waitForUpdatesAsync();
const removeEvent = jasmine.createSpy();
element.addEventListener('remove', removeEvent);
const pageObject = new ChipPageObject(element);
pageObject.clickRemoveButton();
expect(removeEvent).toHaveBeenCalled();
});

it('remove button is not visible when not removable', async () => {
element.removable = false;
await waitForUpdatesAsync();
const pageObject = new ChipPageObject(element);
expect(pageObject.isRemoveButtonVisible()).toBe(false);
});

it('remove button is not visible when disabled', async () => {
element.disabled = true;
await waitForUpdatesAsync();
const pageObject = new ChipPageObject(element);
expect(pageObject.isRemoveButtonVisible()).toBe(false);
});

describe('title overflow', () => {
beforeEach(async () => {
element.style.width = '200px';
await waitForUpdatesAsync();
});

function dispatchEventToChipContent(event: Event): boolean | undefined {
return element
.shadowRoot!.querySelector('.content')!
.dispatchEvent(event);
}

function getChipContentTitle(): string {
return (
element
.shadowRoot!.querySelector('.content')!
.getAttribute('title') ?? ''
);
}

it('sets title when chip text is ellipsized', async () => {
const chipContent = 'a very long value that should get ellipsized due to not fitting within the allocated width';
element.textContent = chipContent;
dispatchEventToChipContent(new MouseEvent('mouseover'));
await waitForUpdatesAsync();
expect(getChipContentTitle()).toBe(chipContent);
});
});
});
6 changes: 6 additions & 0 deletions packages/nimble-components/src/chip/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export const ChipAppearance = {
outline: 'outline',
block: 'block'
} as const;
export type ChipAppearance =
(typeof ChipAppearance)[keyof typeof ChipAppearance];
9 changes: 7 additions & 2 deletions packages/nimble-components/src/label-provider/core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ import {
filterNoResultsLabel,
loadingLabel,
scrollBackwardLabel,
scrollForwardLabel
scrollForwardLabel,
chipRemoveLabel
} from './label-tokens';
import { styles } from '../base/styles';

Expand All @@ -33,7 +34,8 @@ const supportedLabels = {
filterNoResults: filterNoResultsLabel,
loading: loadingLabel,
scrollBackward: scrollBackwardLabel,
scrollForward: scrollForwardLabel
scrollForward: scrollForwardLabel,
chipRemove: chipRemoveLabel
Copy link
Member

Choose a reason for hiding this comment

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

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

} as const;

/**
Expand Down Expand Up @@ -75,6 +77,9 @@ export class LabelProviderCore
@attr({ attribute: 'scroll-forward' })
public scrollForward: string | undefined;

@attr({ attribute: 'chip-remove' })
public chipRemove: string | undefined;

protected override readonly supportedLabels = supportedLabels;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@ export const coreLabelDefaults: { readonly [key in TokenName]: string } = {
filterNoResultsLabel: 'No items found',
loadingLabel: 'Loading…',
scrollBackwardLabel: 'Scroll backward',
scrollForwardLabel: 'Scroll forward'
scrollForwardLabel: 'Scroll forward',
chipRemoveLabel: 'Remove'
};
Loading
Loading