Skip to content
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

Required indicator on radio-group, select, combobox, and text-area #2496

Merged
merged 26 commits into from
Jan 6, 2025
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
16914ba
first pass
mollykreis Dec 10, 2024
1e9a32e
matrix tests + storybook description
mollykreis Dec 10, 2024
e4edfda
format
mollykreis Dec 10, 2024
bccbde5
Change files
mollykreis Dec 10, 2024
f388e3e
Merge branch 'main' into required-indicator
mollykreis Dec 12, 2024
1e913e4
radio group
mollykreis Dec 12, 2024
1d323aa
use shared label template
mollykreis Dec 12, 2024
7bd78b6
rename function
mollykreis Dec 12, 2024
2b481b2
tests
mollykreis Dec 12, 2024
7432453
format
mollykreis Dec 12, 2024
5e6bf30
minor clean up
mollykreis Dec 12, 2024
8bb205d
reduce chromatic churn
mollykreis Dec 12, 2024
59c1770
fix build
mollykreis Dec 12, 2024
ff34b9c
add attribute name to radio-group story
mollykreis Dec 13, 2024
1f80211
remove unnecessary storybook/addon-mdx-gfm
fredvisser Dec 16, 2024
8a7ecd1
remove getAbsolutePath to resolve build issue
fredvisser Dec 16, 2024
2b1c4c4
Merge branch 'main' into required-indicator
mollykreis Jan 2, 2025
d72531c
Fix height of radio-group label when wrapped
mollykreis Jan 2, 2025
15d0256
Merge branch 'main' into required-indicator
mollykreis Jan 3, 2025
101fc8f
rename function
mollykreis Jan 3, 2025
c000902
Update preview.js
mollykreis Jan 3, 2025
4e7a563
update icon styles
mollykreis Jan 3, 2025
e8b34bc
remove width & height of 'display: contents'
mollykreis Jan 3, 2025
1a25eea
Add comment for createRequiredVisibleLabelTemplate
mollykreis Jan 6, 2025
ef453cb
Create story for required-visible pattern
mollykreis Jan 6, 2025
3b4f8d0
format
mollykreis Jan 6, 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 @@
{
mollykreis marked this conversation as resolved.
Show resolved Hide resolved
"type": "minor",
"comment": "Add `required-visible` attribute to radio-group, combobox, select, and text-area",
"packageName": "@ni/nimble-components",
"email": "20542556+mollykreis@users.noreply.github.com",
"dependentChangeType": "patch"
}
5 changes: 4 additions & 1 deletion packages/nimble-components/src/combobox/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import type { AnchoredRegion } from '../anchored-region';
import { template } from './template';
import { FormAssociatedCombobox } from './models/combobox-form-associated';
import type { ListOption } from '../list-option';
import { mixinRequiredVisiblePattern } from '../patterns/required-visible/types';

declare global {
interface HTMLElementTagNameMap {
Expand All @@ -52,7 +53,9 @@ declare global {
* A nimble-styed HTML combobox
*/
export class Combobox
extends mixinErrorPattern(FormAssociatedCombobox)
extends mixinErrorPattern(
mixinRequiredVisiblePattern(FormAssociatedCombobox)
)
implements DropdownPattern {
@attr
public appearance: DropdownAppearance = DropdownAppearance.underline;
Expand Down
2 changes: 2 additions & 0 deletions packages/nimble-components/src/combobox/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {

import { styles as dropdownStyles } from '../patterns/dropdown/styles';
import { styles as errorStyles } from '../patterns/error/styles';
import { styles as requiredVisibleStyles } from '../patterns/required-visible/styles';
import { focusVisible } from '../utilities/style/focus';
import { appearanceBehavior } from '../utilities/style/appearance';
import { DropdownAppearance } from '../select/types';
Expand All @@ -18,6 +19,7 @@ import { userSelectNone } from '../utilities/style/user-select';
export const styles = css`
${dropdownStyles}
${errorStyles}
${requiredVisibleStyles}

:host {
--ni-private-hover-bottom-border-width: 2px;
Expand Down
12 changes: 9 additions & 3 deletions packages/nimble-components/src/combobox/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ import { anchoredRegionTag } from '../anchored-region';
import { DropdownPosition } from '../patterns/dropdown/types';
import { overflow } from '../utilities/directive/overflow';
import { filterNoResultsLabel } from '../label-provider/core/label-tokens';
import { getRequiredVisibleLabelTemplate } from '../patterns/required-visible/template';

const labelTemplate = getRequiredVisibleLabelTemplate(html<Combobox>`
<label part="label" class="label">
<slot></slot>
</label>
`);

/* eslint-disable @typescript-eslint/indent */
// prettier-ignore
Expand All @@ -34,9 +41,7 @@ ComboboxOptions
@focusout="${(x, c) => x.focusoutHandler(c.event as FocusEvent)}"
@keydown="${(x, c) => x.keydownHandler(c.event as KeyboardEvent)}"
>
<label part="label" class="label">
<slot></slot>
</label>
${labelTemplate}
<div class="control" part="control" ${ref('controlWrapper')}>
${startSlotTemplate(context, definition)}
<slot name="control">
Expand All @@ -46,6 +51,7 @@ ComboboxOptions
aria-controls="${x => x.ariaControls}"
aria-disabled="${x => x.ariaDisabled}"
aria-expanded="${x => x.ariaExpanded}"
aria-required="${x => x.requiredVisible}"
aria-haspopup="listbox"
class="selected-value"
part="selected-value"
Expand Down
17 changes: 16 additions & 1 deletion packages/nimble-components/src/combobox/tests/combobox.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ import { parameterizeSpec, parameterizeSuite } from '@ni/jasmine-parameterized';
import { fixture, Fixture } from '../../utilities/tests/fixture';
import { Combobox, comboboxTag } from '..';
import { ComboboxAutocomplete } from '../types';
import { waitForUpdatesAsync } from '../../testing/async-helpers';
import {
processUpdates,
waitForUpdatesAsync
} from '../../testing/async-helpers';
import { checkFullyInViewport } from '../../utilities/tests/intersection-observer';
import { listOptionTag } from '../../list-option';
import { ComboboxPageObject } from '../testing/combobox.pageobject';
Expand Down Expand Up @@ -106,6 +109,18 @@ describe('Combobox', () => {
expect(element.control.getAttribute('disabled')).not.toBeNull();
});

it('should set "aria-required" to true when "required-visible" is true', () => {
element.requiredVisible = true;
processUpdates();
expect(element.control.getAttribute('aria-required')).toBe('true');
});

it('should set "aria-required" to false when "required-visible" is false', () => {
element.requiredVisible = false;
processUpdates();
expect(element.control.getAttribute('aria-required')).toBe('false');
});

it('should forward value property to inner control', async () => {
element.value = 'foo';
await waitForUpdatesAsync();
Expand Down
1 change: 1 addition & 0 deletions packages/nimble-components/src/icon-base/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export const styles = css`
}

.icon svg {
display: block;
mollykreis marked this conversation as resolved.
Show resolved Hide resolved
mollykreis marked this conversation as resolved.
Show resolved Hide resolved
fill: ${iconColor};
width: 100%;
height: 100%;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { css } from '@microsoft/fast-element';
import { smallPadding } from '../../theme-provider/design-tokens';

export const styles = css`
.annotated-label {
display: flex;
flex-direction: row;
}

.required-icon {
flex: none;
width: 5px;
height: 5px;
margin-top: 3px;
margin-left: ${smallPadding};
}
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { ViewTemplate, html, when } from '@microsoft/fast-element';
import { iconAsteriskTag } from '../../icons/asterisk';
import type { RequiredVisiblePattern } from './types';

/* eslint-disable @typescript-eslint/indent */
export function getRequiredVisibleLabelTemplate(
mollykreis marked this conversation as resolved.
Show resolved Hide resolved
labelTemplate: ViewTemplate<RequiredVisiblePattern>
): ViewTemplate<RequiredVisiblePattern> {
return html`
<div class="annotated-label">
${labelTemplate}
${when(
x => x.requiredVisible,
html`
<${iconAsteriskTag} class="required-icon" severity="error"></${iconAsteriskTag}>
`
)}
</div>
`;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import type { FoundationElement } from '@microsoft/fast-foundation';

/**
* A page object for the elements that use the required-visible mixin.
*/
export class RequiredVisiblePatternPageObject {
public constructor(private readonly element: FoundationElement) {}

public isRequiredVisibleIconVisible(): boolean {
const icon = this.getRequiredVisibleIcon();
if (!icon) {
return false;
}
return getComputedStyle(icon).display !== 'none';
}

private getRequiredVisibleIcon(): HTMLElement | null {
return this.element.shadowRoot!.querySelector('.required-icon');
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { customElement, html } from '@microsoft/fast-element';
import { FoundationElement } from '@microsoft/fast-foundation';
import {
Fixture,
fixture,
uniqueElementName
} from '../../../utilities/tests/fixture';
import { mixinRequiredVisiblePattern } from '../types';
import { styles } from '../styles';
import { getRequiredVisibleLabelTemplate } from '../template';
import { processUpdates } from '../../../testing/async-helpers';
import { RequiredVisiblePatternPageObject } from '../testing/required-visible-pattern.pageobject';

const labelTemplate = getRequiredVisibleLabelTemplate(
html`<slot name="label"></slot>`
);
const elementName = uniqueElementName();
@customElement({
name: elementName,
template: html`${labelTemplate}`,
styles
})
class TestErrorPatternElement extends mixinRequiredVisiblePattern(
FoundationElement
) {}

async function setup(): Promise<Fixture<TestErrorPatternElement>> {
return await fixture(elementName);
}

describe('RequiredVisiblePatternMixin', () => {
let element: TestErrorPatternElement;
let connect: () => Promise<void>;
let disconnect: () => Promise<void>;
let pageObject: RequiredVisiblePatternPageObject;

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

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

it('defaults requiredVisible to false', () => {
expect(element.requiredVisible).toBeFalse();
});

it('shows icon when requiredVisible is true', () => {
element.requiredVisible = true;
processUpdates();
expect(pageObject.isRequiredVisibleIconVisible()).toBeTrue();
});

it('does not show icon when requiredVisible is false', () => {
element.requiredVisible = false;
processUpdates();
expect(pageObject.isRequiredVisibleIconVisible()).toBeFalse();
});

it('uses boolean "required-visible" attribute to set requiredVisible', () => {
element.setAttribute('required-visible', '');
processUpdates();
expect(element.requiredVisible).toBeTrue();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { attr, FASTElement } from '@microsoft/fast-element';

export interface RequiredVisiblePattern {
/**
* Whether or not to show the required appearance of the control
*/
requiredVisible: boolean;
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
type FASTElementConstructor = abstract new (...args: any[]) => FASTElement;

// As the returned class is internal to the function, we can't write a signature that uses is directly, so rely on inference
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types, @typescript-eslint/explicit-function-return-type
export function mixinRequiredVisiblePattern<
TBase extends FASTElementConstructor
>(base: TBase) {
/**
* The Mixin that provides the requiredVisible property and required-visible attribute
* to a component.
*/
abstract class RequiredVisibleElement
extends base
implements RequiredVisiblePattern {
/*
* Show the required appearance of the control
*/
public requiredVisible = false;
}

attr({ attribute: 'required-visible', mode: 'boolean' })(
RequiredVisibleElement.prototype,
'requiredVisible'
);
return RequiredVisibleElement;
}
5 changes: 4 additions & 1 deletion packages/nimble-components/src/radio-group/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { Orientation } from '@microsoft/fast-web-utilities';
import { styles } from './styles';
import { template } from './template';
import { mixinErrorPattern } from '../patterns/error/types';
import { mixinRequiredVisiblePattern } from '../patterns/required-visible/types';

declare global {
interface HTMLElementTagNameMap {
Expand All @@ -18,7 +19,9 @@ export { Orientation };
/**
* A nimble-styled grouping element for radio buttons
*/
export class RadioGroup extends mixinErrorPattern(FoundationRadioGroup) {}
export class RadioGroup extends mixinErrorPattern(
mixinRequiredVisiblePattern(FoundationRadioGroup)
) {}

const nimbleRadioGroup = RadioGroup.compose({
baseName: 'radio-group',
Expand Down
2 changes: 2 additions & 0 deletions packages/nimble-components/src/radio-group/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@ import {
standardPadding
} from '../theme-provider/design-tokens';
import { styles as errorStyles } from '../patterns/error/styles';
import { styles as requiredVisibleStyles } from '../patterns/required-visible/styles';

export const styles = css`
${display('inline-block')}
${errorStyles}
${requiredVisibleStyles}

.positioning-region {
display: flex;
Expand Down
8 changes: 7 additions & 1 deletion packages/nimble-components/src/radio-group/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,25 @@ import { Orientation } from '@microsoft/fast-web-utilities';
import type { RadioGroup } from '.';
import { errorTextTemplate } from '../patterns/error/template';
import { iconExclamationMarkTag } from '../icons/exclamation-mark';
import { getRequiredVisibleLabelTemplate } from '../patterns/required-visible/template';

const labelTemplate = getRequiredVisibleLabelTemplate(
html<RadioGroup>`<slot name="label"></slot>`
);

/* eslint-disable @typescript-eslint/indent */
export const template = html<RadioGroup>`
<template
role="radiogroup"
aria-disabled="${x => x.disabled}"
aria-readonly="${x => x.readOnly}"
aria-required="${x => x.requiredVisible}"
@click="${(x, c) => x.clickHandler(c.event as MouseEvent)}"
@keydown="${(x, c) => x.keydownHandler(c.event as KeyboardEvent)}"
@focusout="${(x, c) => x.focusOutHandler(c.event as FocusEvent)}"
>
<div class="label-container">
<slot name="label"></slot>
${labelTemplate}
<${iconExclamationMarkTag}
severity="error"
class="error-icon"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,43 @@
import { html } from '@microsoft/fast-element';
import { RadioGroup, radioGroupTag } from '..';
import { Fixture, fixture } from '../../utilities/tests/fixture';
import { processUpdates } from '../../testing/async-helpers';

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

describe('Radio Group', () => {
let element: RadioGroup;
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(radioGroupTag)).toBeInstanceOf(
RadioGroup
);
});

it('should set "aria-required" to true when "required-visible" is true', () => {
element.requiredVisible = true;
processUpdates();
expect(element.getAttribute('aria-required')).toBe('true');
});

it('should set "aria-required" to false when "required-visible" is false', () => {
element.requiredVisible = false;
processUpdates();
expect(element.getAttribute('aria-required')).toBe('false');
});
});
Loading
Loading