Skip to content
Open
Show file tree
Hide file tree
Changes from 14 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"
}
84 changes: 84 additions & 0 deletions packages/nimble-components/src/chip/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
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');
}
}

public keyDownHandler(event: KeyboardEvent): boolean {
if (
(event.key === 'Delete' || event.key === 'Backspace')
&& this.removable
) {
this.handleRemoveClick();
event.stopPropagation();
event.preventDefault();
}
return true;
}
}
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,
controlSlimHeight,
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: ${controlSlimHeight};
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);
}
`
)
);
48 changes: 48 additions & 0 deletions packages/nimble-components/src/chip/template.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
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';

export const template: FoundationElementTemplate<
ViewTemplate<Chip>,
ChipOptions
> = (context, definition) => html<Chip>`
<template
aria-disabled="${x => x.disabled}"
@keydown="${(x, c) => x.keyDownHandler(c.event as KeyboardEvent)}"
>
${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,
html<Chip>`
<${buttonTag}
class="remove-button"
content-hidden
appearance="ghost"
@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;

`;
21 changes: 21 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,21 @@
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 clickRemoveButton(): void {
const removeButton = this.chipElement.shadowRoot?.querySelector<Button>(
'.remove-button'
);
if (removeButton) {
removeButton.click();
} else {
throw new Error('Remove button not found');
}
}
}
71 changes: 71 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,71 @@
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 { sendKeyDownEvent } from '../../utilities/testing/component';
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('pressing Delete key raises remove event when removable is true', async () => {
element.removable = true;
const removeEvent = jasmine.createSpy();
element.addEventListener('remove', removeEvent);
await sendKeyDownEvent(element, 'Delete');
expect(removeEvent).toHaveBeenCalled();
});

it('pressing Backspace key raises remove event when removable is true', async () => {
element.removable = true;
const removeEvent = jasmine.createSpy();
element.addEventListener('remove', removeEvent);
await sendKeyDownEvent(element, 'Backspace');
expect(removeEvent).toHaveBeenCalled();
});

it('pressing Delete key does not raise remove event when removable is false', async () => {
element.removable = false;
const removeEvent = jasmine.createSpy();
element.addEventListener('remove', removeEvent);
await sendKeyDownEvent(element, 'Delete');
expect(removeEvent).not.toHaveBeenCalled();
});

it('pressing Backspace key does not raise remove event when removable is false', async () => {
element.removable = false;
const removeEvent = jasmine.createSpy();
element.addEventListener('remove', removeEvent);
await sendKeyDownEvent(element, 'Backspace');
expect(removeEvent).not.toHaveBeenCalled();
});
});
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