Skip to content

Commit

Permalink
Feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
m-akinc committed May 20, 2024
1 parent dcfc5df commit 36a01b8
Show file tree
Hide file tree
Showing 6 changed files with 45 additions and 25 deletions.
12 changes: 6 additions & 6 deletions packages/nimble-components/src/button/tests/button.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,8 @@ import { fixture, type Fixture } from '../../utilities/tests/fixture';
import { waitForUpdatesAsync } from '../../testing/async-helpers';

describe('Button', () => {
async function setup(tabIndex?: string): Promise<Fixture<Button>> {
return fixture<Button>(
html`<${buttonTag} ${tabIndex ? `tabindex=${tabIndex}` : ''}></${buttonTag}>`
);
async function setup(): Promise<Fixture<Button>> {
return fixture<Button>(html`<${buttonTag}></${buttonTag}>`);
}

it('should export its tag', () => {
Expand All @@ -30,7 +28,8 @@ describe('Button', () => {
});

it('should set the `tabindex` attribute on the internal button when provided', async () => {
const { element, connect, disconnect } = await setup('-1');
const { element, connect, disconnect } = await setup();
element.setAttribute('tabindex', '-1');
await connect();

const innerButton = element.shadowRoot!.querySelector('button')!;
Expand All @@ -41,7 +40,8 @@ describe('Button', () => {
});

it('should clear the `tabindex` attribute on the internal button when cleared from the host', async () => {
const { element, connect, disconnect } = await setup('-1');
const { element, connect, disconnect } = await setup();
element.setAttribute('tabindex', '-1');
await connect();

element.removeAttribute('tabindex');
Expand Down
8 changes: 8 additions & 0 deletions packages/nimble-components/src/checkbox/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ export class Checkbox extends FoundationCheckbox {
*/
@attr({ attribute: 'tabindex', converter: nullableNumberConverter })
public override tabIndex!: number;

/**
* @internal
*/
public getEffectiveTabIndex(): number | null {
// prettier-ignore
return this.disabled ? null : (this.tabIndex ?? 0);
}
}

const nimbleCheckbox = Checkbox.compose<CheckboxOptions>({
Expand Down
3 changes: 1 addition & 2 deletions packages/nimble-components/src/checkbox/template.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
/* eslint-disable no-restricted-syntax */
import { html, slotted, type ViewTemplate } from '@microsoft/fast-element';
import type {
CheckboxOptions,
Expand All @@ -16,7 +15,7 @@ CheckboxOptions
aria-required="${x => x.required}"
aria-disabled="${x => x.disabled}"
aria-readonly="${x => x.readOnly}"
tabindex="${x => (x.disabled ? null : x.tabIndex ?? 0)}"
tabindex="${x => x.getEffectiveTabIndex()}"
@keypress="${(x, c) => x.keypressHandler(c.event as KeyboardEvent)}"
@click="${(x, c) => x.clickHandler(c.event as MouseEvent)}"
class="${x => (x.readOnly ? 'readonly' : '')} ${x => (x.checked ? 'checked' : '')} ${x => (x.indeterminate ? 'indeterminate' : '')}"
Expand Down
12 changes: 6 additions & 6 deletions packages/nimble-components/src/checkbox/tests/checkbox.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,8 @@ import { fixture, type Fixture } from '../../utilities/tests/fixture';
import { waitForUpdatesAsync } from '../../testing/async-helpers';

describe('Checkbox', () => {
async function setup(tabIndex?: string): Promise<Fixture<Checkbox>> {
return fixture<Checkbox>(
html`<${checkboxTag} ${tabIndex ? `tabindex=${tabIndex}` : ''}></${checkboxTag}>`
);
async function setup(): Promise<Fixture<Checkbox>> {
return fixture<Checkbox>(html`<${checkboxTag}></${checkboxTag}>`);
}

it('should export its tag', () => {
Expand All @@ -21,7 +19,8 @@ describe('Checkbox', () => {
});

it('should honor provided `tabindex` value', async () => {
const { element, connect, disconnect } = await setup('-1');
const { element, connect, disconnect } = await setup();
element.setAttribute('tabindex', '-1');
await connect();

expect(element.getAttribute('tabindex')).toEqual('-1');
Expand All @@ -31,7 +30,8 @@ describe('Checkbox', () => {
});

it('should default `tabindex` back to 0 when removed', async () => {
const { element, connect, disconnect } = await setup('-1');
const { element, connect, disconnect } = await setup();
element.setAttribute('tabindex', '-1');
await connect();

element.removeAttribute('tabindex');
Expand Down
8 changes: 8 additions & 0 deletions packages/nimble-components/src/toggle-button/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,14 @@ export class ToggleButton extends FoundationSwitch implements ButtonPattern {

/** @internal */
public readonly control!: HTMLElement;

/**
* @internal
*/
public getEffectiveTabIndex(): number | null {
// prettier-ignore
return this.disabled ? null : (this.tabIndex ?? 0);
}
}
applyMixins(ToggleButton, StartEnd, DelegatesARIAButton);
export interface ToggleButton extends StartEnd, DelegatesARIAButton {}
Expand Down
27 changes: 16 additions & 11 deletions packages/nimble-components/src/toggle-button/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,35 @@ import {
} from '@microsoft/fast-foundation';
import type { ToggleButton } from '.';

/* eslint-disable @typescript-eslint/indent */
// prettier-ignore
export const template: FoundationElementTemplate<
ViewTemplate<ToggleButton>,
ButtonOptions
> = (context, definition) => html<ToggleButton>`
<div
role="button"
part="control"
tabindex="${x => (x.disabled ? null : x.tabIndex ?? 0)}"
tabindex="${x => x.getEffectiveTabIndex()}"
@keypress="${(x, c) => x.keypressHandler(c.event as KeyboardEvent)}"
@click="${(x, c) => x.clickHandler(c.event as MouseEvent)}"
class="control ${x => (x.checked ? 'checked' : '')}"
?disabled="${x => x.disabled}"
${
'' /* Configure aria-disabled, aria-readonly, and aria-pressed based on the
toggle button's state to keep the ARIA attributes consistent with the component's
state without a client having to configure ARIA attributes directly */
}
${''
/**
* Configure aria-disabled, aria-readonly, and aria-pressed based on the
* toggle button's state to keep the ARIA attributes consistent with the component's
* state without a client having to configure ARIA attributes directly
*/
}
aria-disabled="${x => x.disabled}"
aria-readonly="${x => x.readOnly}"
aria-pressed="${x => x.checked}"
${
'' /* Configure all other ARIA attributes based on the aria attributes
configured on the toggle button */
}
${''
/**
* Configure all other ARIA attributes based on the aria attributes
* configured on the toggle button
*/
}
aria-atomic="${x => x.ariaAtomic}"
aria-busy="${x => x.ariaBusy}"
aria-controls="${x => x.ariaControls}"
Expand Down

0 comments on commit 36a01b8

Please sign in to comment.