Skip to content

Commit

Permalink
fix(sbb-radio-button-group): sync radios synchronously (#3323)
Browse files Browse the repository at this point in the history
  • Loading branch information
jeripeierSBB authored and github-actions committed Dec 20, 2024
1 parent 2f9abcb commit 26c7b47
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 97 deletions.
6 changes: 2 additions & 4 deletions src/elements/core/mixins/form-associated-mixin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,9 @@ import type { AbstractConstructor } from './constructor.js';

export declare abstract class SbbFormAssociatedMixinType<V = string> {
public get form(): HTMLFormElement | null;
public get name(): string;
public set name(value: string);
public accessor name: string;
public get type(): string;
public get value(): V | null;
public set value(value: V | null);
public accessor value: V | null;

public get validity(): ValidityState;
public get validationMessage(): string;
Expand Down
76 changes: 39 additions & 37 deletions src/elements/core/mixins/form-associated-radio-button-mixin.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import type { LitElement, PropertyValues } from 'lit';
import { isServer, type LitElement, type PropertyValues } from 'lit';
import { property } from 'lit/decorators.js';

import { getNextElementIndex, interactivityChecker, isArrowKeyPressed } from '../a11y.js';
import { SbbConnectedAbortController } from '../controllers.js';
import { forceType } from '../decorators.js';

import type { Constructor } from './constructor.js';
import { SbbDisabledMixin, type SbbDisabledMixinType } from './disabled-mixin.js';
Expand Down Expand Up @@ -31,9 +30,9 @@ export declare class SbbFormAssociatedRadioButtonMixinType
extends SbbFormAssociatedMixinType
implements Partial<SbbDisabledMixinType>, Partial<SbbRequiredMixinType>
{
public checked: boolean;
public disabled: boolean;
public required: boolean;
public accessor checked: boolean;
public accessor disabled: boolean;
public accessor required: boolean;

protected associatedRadioButtons?: Set<SbbFormAssociatedRadioButtonMixinType>;
/** @deprecated No longer used internally. */
Expand Down Expand Up @@ -65,9 +64,30 @@ export const SbbFormAssociatedRadioButtonMixin = <T extends Constructor<LitEleme
/**
* Whether the radio button is checked.
*/
@forceType()
@property({ type: Boolean })
public accessor checked: boolean = false;
public set checked(value: boolean) {
this._checked = !!value;

this.toggleAttribute('data-checked', this.checked);
this.internals.ariaChecked = this.checked.toString();
this.updateFormValue();
this._synchronizeGroupState();
}
public get checked(): boolean {
return this._checked;
}
private _checked: boolean = false;

public override set name(value: string) {
super.name = value;

this._disconnectFromRegistry();
this._connectToRegistry();
this._synchronizeGroupState();
}
public override get name(): string {
return super.name;
}

/**
* Form type of element.
Expand Down Expand Up @@ -98,6 +118,7 @@ export const SbbFormAssociatedRadioButtonMixin = <T extends Constructor<LitEleme
public override connectedCallback(): void {
super.connectedCallback();
this._connectToRegistry();
this._synchronizeGroupState();
}

public override disconnectedCallback(): void {
Expand Down Expand Up @@ -131,26 +152,6 @@ export const SbbFormAssociatedRadioButtonMixin = <T extends Constructor<LitEleme
protected override willUpdate(changedProperties: PropertyValues<this>): void {
super.willUpdate(changedProperties);

// On 'name' change, move 'this' to the new registry
if (changedProperties.has('name')) {
this._disconnectFromRegistry();
this._connectToRegistry();
if (this.checked) {
this._deselectGroupedRadios();
}
this.updateFocusableRadios();
}

if (changedProperties.has('checked')) {
this.toggleAttribute('data-checked', this.checked);
this.internals.ariaChecked = this.checked.toString();
this.updateFormValueOnCheckedChange();
if (this.checked) {
this._deselectGroupedRadios();
}
this.updateFocusableRadios();
}

if (changedProperties.has('disabled')) {
this.updateFocusableRadios();
}
Expand All @@ -168,18 +169,12 @@ export const SbbFormAssociatedRadioButtonMixin = <T extends Constructor<LitEleme
*/
protected override updateFormValue(): void {
if (this.checked) {
this.internals.setFormValue(this.value);
this.internals.setFormValue(this.value, this.value);
} else {
this.internals.setFormValue(null);
}
}

/**
* Called on `checked` change
* If 'checked', update the value. Otherwise, reset it.
*/
protected updateFormValueOnCheckedChange(): void {
this.internals.setFormValue(this.checked ? this.value : null);
}

/**
* Only a single radio should be focusable in the group. Defined as:
* - the checked radio;
Expand Down Expand Up @@ -221,11 +216,18 @@ export const SbbFormAssociatedRadioButtonMixin = <T extends Constructor<LitEleme
this.dispatchEvent(new Event('change', { bubbles: true }));
}

private _synchronizeGroupState(): void {
if (this.checked) {
this._deselectGroupedRadios();
}
this.updateFocusableRadios();
}

/**
* Add `this` to the radioButton registry
*/
private _connectToRegistry(): void {
if (!this.name) {
if (!this.name || isServer) {
return;
}

Expand Down
40 changes: 1 addition & 39 deletions src/elements/radio-button/common/radio-button-common.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,7 @@
import type { LitElement, PropertyValues } from 'lit';
import type { LitElement } from 'lit';
import { property } from 'lit/decorators.js';

import { setModalityOnNextFocus } from '../../core/a11y.js';
import { EventEmitter } from '../../core/eventing.js';
import type {
SbbCheckedStateChange,
SbbDisabledStateChange,
SbbStateChange,
} from '../../core/interfaces.js';
import {
type AbstractConstructor,
type Constructor,
Expand All @@ -18,11 +12,6 @@ import type { SbbRadioButtonGroupElement } from '../radio-button-group.js';

export type SbbRadioButtonSize = 'xs' | 's' | 'm';

export type SbbRadioButtonStateChange = Extract<
SbbStateChange,
SbbDisabledStateChange | SbbCheckedStateChange
>;

export declare class SbbRadioButtonCommonElementMixinType extends SbbFormAssociatedRadioButtonMixinType {
public get allowEmptySelection(): boolean;
public set allowEmptySelection(boolean);
Expand All @@ -41,7 +30,6 @@ export const SbbRadioButtonCommonElementMixin = <T extends Constructor<LitElemen
public static readonly events = {
change: 'change',
input: 'input',
stateChange: 'stateChange',
} as const;

/**
Expand All @@ -64,17 +52,6 @@ export const SbbRadioButtonCommonElementMixin = <T extends Constructor<LitElemen
}
private _group: SbbRadioButtonGroupElement | null = null;

/**
* @internal
* Internal event that emits whenever the state of the radio option
* in relation to the parent selection panel changes.
*/
private _stateChange: EventEmitter<SbbRadioButtonStateChange> = new EventEmitter(
this,
SbbRadioButtonCommonElement.events.stateChange,
{ bubbles: true },
);

public constructor() {
super();
this.addEventListener?.('click', (e) => this._handleClick(e));
Expand Down Expand Up @@ -107,21 +84,6 @@ export const SbbRadioButtonCommonElementMixin = <T extends Constructor<LitElemen
}
}

protected override willUpdate(changedProperties: PropertyValues<this>): void {
super.willUpdate(changedProperties);

if (changedProperties.has('checked')) {
if (this.checked !== changedProperties.get('checked')!) {
this._stateChange.emit({ type: 'checked', checked: this.checked });
}
}
if (changedProperties.has('disabled')) {
if (this.disabled !== changedProperties.get('disabled')!) {
this._stateChange.emit({ type: 'disabled', disabled: this.disabled });
}
}
}

protected override isDisabledExternally(): boolean {
return this.group?.disabled ?? false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,25 @@ import '../radio-button-panel.js';

describe('events', () => {
it('dispatches event on radio change', async () => {
radios[0].checked = true;
const radio = radios[1];
const changeSpy = new EventSpy('change');
const inputSpy = new EventSpy('input');
const changeSpy = new EventSpy('change', element);
const inputSpy = new EventSpy('input', element);

element.addEventListener(
'change',
() => {
expect(element.value).to.be.equal('Value two');
expect(
Array.from(
element.querySelectorAll<SbbRadioButtonElement | SbbRadioButtonPanelElement>(
selector,
),
).filter((radio) => radio.checked).length,
).to.be.equal(1);
},
{ once: true },
);

radio.click();
await waitForLitRender(element);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class SbbRadioButtonGroupElement extends SbbDisabledMixin(LitElement) {
if (!this._didLoad) {
return;
}
if (!val) {
if (val == null) {
this.radioButtons.forEach((r) => (r.checked = false));
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ import { customElement, property } from 'lit/decorators.js';

import { getOverride, slotState } from '../../core/decorators.js';
import { isLean } from '../../core/dom.js';
import { EventEmitter } from '../../core/eventing.js';
import type {
SbbCheckedStateChange,
SbbDisabledStateChange,
SbbStateChange,
} from '../../core/interfaces.js';
import {
panelCommonStyle,
SbbPanelMixin,
Expand All @@ -20,6 +26,11 @@ import { radioButtonCommonStyle, SbbRadioButtonCommonElementMixin } from '../com

import '../../screen-reader-only.js';

export type SbbRadioButtonStateChange = Extract<
SbbStateChange,
SbbDisabledStateChange | SbbCheckedStateChange
>;

/**
/**
* It displays a radio button enhanced with the panel design.
Expand All @@ -42,10 +53,10 @@ class SbbRadioButtonPanelElement extends SbbPanelMixin(

// TODO: fix using ...super.events requires: https://github.com/sbb-design-systems/lyne-components/issues/2600
public static readonly events = {
stateChange: 'stateChange',
change: 'change',
input: 'input',
panelConnected: 'panelConnected',
stateChange: 'stateChange',
} as const;

/**
Expand All @@ -58,6 +69,17 @@ class SbbRadioButtonPanelElement extends SbbPanelMixin(

private _hasSelectionExpansionPanelElement: boolean = false;

/**
* @internal
* Internal event that emits whenever the state of the radio option
* in relation to the parent selection panel changes.
*/
private _stateChange: EventEmitter<SbbRadioButtonStateChange> = new EventEmitter(
this,
SbbRadioButtonPanelElement.events.stateChange,
{ bubbles: true },
);

public override connectedCallback(): void {
super.connectedCallback();
this._hasSelectionExpansionPanelElement = !!this.closest?.('sbb-selection-expansion-panel');
Expand All @@ -68,6 +90,16 @@ class SbbRadioButtonPanelElement extends SbbPanelMixin(

if (changedProperties.has('checked')) {
this.toggleAttribute('data-checked', this.checked);

if (this.checked !== changedProperties.get('checked')!) {
this._stateChange.emit({ type: 'checked', checked: this.checked });
}
}

if (changedProperties.has('disabled')) {
if (this.disabled !== changedProperties.get('disabled')!) {
this._stateChange.emit({ type: 'disabled', disabled: this.disabled });
}
}
}

Expand Down
26 changes: 13 additions & 13 deletions src/elements/radio-button/radio-button/radio-button.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,48 +22,48 @@ describe(`sbb-radio-button`, () => {
});

it('selects radio on click', async () => {
const stateChange = new EventSpy(SbbRadioButtonElement.events.stateChange);
const change = new EventSpy(SbbRadioButtonElement.events.change);

element.click();
await waitForLitRender(element);

expect(element).to.have.attribute('data-checked');
expect(element.checked).to.be.true;
await stateChange.calledOnce();
expect(stateChange.count).to.be.equal(1);
await change.calledOnce();
expect(change.count).to.be.equal(1);
});

it('does not deselect radio if already checked', async () => {
const stateChange = new EventSpy(SbbRadioButtonElement.events.stateChange);
const change = new EventSpy(SbbRadioButtonElement.events.change);

element.click();
await waitForLitRender(element);
expect(element.checked).to.be.true;
await stateChange.calledOnce();
expect(stateChange.count).to.be.equal(1);
await change.calledOnce();
expect(change.count).to.be.equal(1);

element.click();
await waitForLitRender(element);
expect(element.checked).to.be.true;
await stateChange.calledOnce();
expect(stateChange.count).to.be.equal(1);
await change.calledOnce();
expect(change.count).to.be.equal(1);
});

it('allows empty selection', async () => {
const stateChange = new EventSpy(SbbRadioButtonElement.events.stateChange);
const change = new EventSpy(SbbRadioButtonElement.events.change);

element.allowEmptySelection = true;
element.click();
await waitForLitRender(element);
expect(element.checked).to.be.true;
await stateChange.calledOnce();
expect(stateChange.count).to.be.equal(1);
await change.calledOnce();
expect(change.count).to.be.equal(1);

element.click();
await waitForLitRender(element);
expect(element.checked).to.be.false;
await stateChange.calledTimes(2);
expect(stateChange.count).to.be.equal(2);
await change.calledTimes(2);
expect(change.count).to.be.equal(2);
});

it('should convert falsy to false', async () => {
Expand Down

0 comments on commit 26c7b47

Please sign in to comment.