-
Notifications
You must be signed in to change notification settings - Fork 10
Nimble chip #2689
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Nimble chip #2689
Changes from 14 commits
bae0a03
a716932
59e3f91
aef1b17
0414761
2325b54
757a50b
a2fdd04
ce0889c
c864b5d
9c318f4
4c927df
90dfa96
00f66c5
1fbc342
3461d3c
b5d2016
6cffc95
70d61ad
c13e138
5b80615
fa47ea7
1561b41
dd5643a
271b05b
9441764
1a8bafc
42737f5
e0615c1
473bce2
b45c0b0
220d3f8
6c35546
311377c
a638378
6acf509
128fbcb
31f79ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @atmgrifter00 still intend to get this pushed through? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
} |
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) | ||
atmgrifter00 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
*/ | ||
export class Chip extends FoundationElement { | ||
@attr({ mode: 'boolean' }) | ||
public removable = false; | ||
|
||
@attr({ mode: 'boolean' }) | ||
public disabled = false; | ||
|
||
@attr() | ||
public appearance: ChipAppearance = 'outline'; | ||
atmgrifter00 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
/** @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}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will the |
||
} | ||
|
||
/** @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>({ | ||
atmgrifter00 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
baseName: 'chip', | ||
template, | ||
styles | ||
}); | ||
|
||
DesignSystem.getOrCreate().withPrefix('nimble').register(nimbleChip()); | ||
export const chipTag = 'nimble-chip'; |
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; | ||
atmgrifter00 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
background-color: transparent; | ||
border: ${borderWidth} solid rgba(${actionRgbPartialColor}, 0.3); | ||
atmgrifter00 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
border-radius: 4px; | ||
justify-content: center; | ||
align-items: center; | ||
background-repeat: no-repeat; | ||
${ | ||
/* | ||
atmgrifter00 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like |
||
:host([disabled]) slot[name='start']::slotted(*), | ||
:host([disabled]) slot[name='end']::slotted(*), | ||
atmgrifter00 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
:host([disabled]) .remove-button { | ||
atmgrifter00 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
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( | ||
|
||
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); | ||
} | ||
` | ||
) | ||
); |
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} | ||
atmgrifter00 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
class="remove-button" | ||
content-hidden | ||
appearance="ghost" | ||
@click="${x => x.handleRemoveClick()}" | ||
> | ||
<${iconTimesTag} slot="start"></${iconTimesTag}> | ||
${x => x.removeButtonContent} | ||
</${buttonTag}> | ||
` | ||
)} | ||
</template> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
`; |
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'); | ||
} | ||
} | ||
} |
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', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 () => { | ||
atmgrifter00 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
element.removable = true; | ||
await waitForUpdatesAsync(); | ||
const removeEvent = jasmine.createSpy(); | ||
element.addEventListener('remove', removeEvent); | ||
const pageObject = new ChipPageObject(element); | ||
pageObject.clickRemoveButton(); | ||
atmgrifter00 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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(); | ||
}); | ||
}); |
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]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,7 +12,8 @@ import { | |
filterNoResultsLabel, | ||
loadingLabel, | ||
scrollBackwardLabel, | ||
scrollForwardLabel | ||
scrollForwardLabel, | ||
chipRemoveLabel | ||
} from './label-tokens'; | ||
import { styles } from '../base/styles'; | ||
|
||
|
@@ -33,7 +34,8 @@ const supportedLabels = { | |
filterNoResults: filterNoResultsLabel, | ||
loading: loadingLabel, | ||
scrollBackward: scrollBackwardLabel, | ||
scrollForward: scrollForwardLabel | ||
scrollForward: scrollForwardLabel, | ||
chipRemove: chipRemoveLabel | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can just go with |
||
} as const; | ||
|
||
/** | ||
|
@@ -75,6 +77,9 @@ export class LabelProviderCore | |
@attr({ attribute: 'scroll-forward' }) | ||
public scrollForward: string | undefined; | ||
|
||
@attr({ attribute: 'chip-remove' }) | ||
public chipRemove: string | undefined; | ||
jattasNI marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
protected override readonly supportedLabels = supportedLabels; | ||
} | ||
|
||
|
Uh oh!
There was an error while loading. Please reload this page.