From 5f4b4c15b91ac349dd35003b380bf69c91c22515 Mon Sep 17 00:00:00 2001 From: Jeremias Peier Date: Wed, 20 Nov 2024 10:07:20 +0100 Subject: [PATCH 1/2] fix(sbb-alert, sbb-alert-group): remove dismissal event and method BREAKING CHANGE: The deprecated `dismissalRequested` event and requestDismissal() method of `sbb-alert` have been removed. The `sbb-alert` handles its closing and DOM removal on his own. If the closing should be prevented, the willClose event can be canceled. The `didDismissAlert` of the `sbb-alert-group` has been removed. As alternative, consumers can listen to the `didClose` event of an `sbb-alert`. --- .../alert/alert-group/alert-group.spec.ts | 48 ++++++++----------- .../alert/alert-group/alert-group.stories.ts | 2 +- src/elements/alert/alert-group/alert-group.ts | 16 ------- src/elements/alert/alert-group/readme.md | 9 ++-- src/elements/alert/alert/alert.spec.ts | 29 +++++++++-- src/elements/alert/alert/alert.stories.ts | 22 ++------- src/elements/alert/alert/alert.ts | 23 +-------- src/elements/alert/alert/readme.md | 26 ++++------ src/elements/notification/notification.ts | 3 +- 9 files changed, 65 insertions(+), 113 deletions(-) diff --git a/src/elements/alert/alert-group/alert-group.spec.ts b/src/elements/alert/alert-group/alert-group.spec.ts index 1cde5ee6a1..3724335046 100644 --- a/src/elements/alert/alert-group/alert-group.spec.ts +++ b/src/elements/alert/alert-group/alert-group.spec.ts @@ -5,12 +5,10 @@ import { html } from 'lit/static-html.js'; import type { SbbTransparentButtonElement } from '../../button.js'; import { fixture } from '../../core/testing/private.js'; import { waitForCondition, EventSpy, waitForLitRender } from '../../core/testing.js'; -import type { SbbAlertElement } from '../alert.js'; +import { SbbAlertElement } from '../alert.js'; import { SbbAlertGroupElement } from './alert-group.js'; -import '../alert.js'; - describe(`sbb-alert-group`, () => { let element: SbbAlertGroupElement; @@ -26,12 +24,18 @@ describe(`sbb-alert-group`, () => { accessibility-title="${accessibilityTitle}" accessibility-title-level="${accessibilityTitleLevel}" > - First - Second + First + Second `); - const didDismissAlertSpy = new EventSpy(SbbAlertGroupElement.events.didDismissAlert); const emptySpy = new EventSpy(SbbAlertGroupElement.events.empty); + const alert1 = element.querySelector('sbb-alert#alert1')!; + const alert2 = element.querySelector('sbb-alert#alert2')!; + const alert1ClosedEventSpy = new EventSpy(SbbAlertElement.events.didClose, alert1); + const alert2ClosedEventSpy = new EventSpy(SbbAlertElement.events.didClose, alert2); + + await new EventSpy(SbbAlertElement.events.didOpen, alert1).calledOnce(); + await new EventSpy(SbbAlertElement.events.didOpen, alert2).calledOnce(); // Then two alerts should be rendered and accessibility title should be displayed expect(element.querySelectorAll('sbb-alert').length).to.be.equal(2); @@ -40,61 +44,47 @@ describe(`sbb-alert-group`, () => { expect(alertGroupTitle.localName).to.be.equal(`h${accessibilityTitleLevel}`); // When clicking on close button of the first alert - const closeButton = element - .querySelector('sbb-alert')! - .shadowRoot!.querySelector( - '.sbb-alert__close-button-wrapper sbb-transparent-button', - )!; + const closeButton = alert1.shadowRoot!.querySelector( + '.sbb-alert__close-button-wrapper sbb-transparent-button', + )!; closeButton.focus(); closeButton.click(); await waitForLitRender(element); + await alert1ClosedEventSpy.calledOnce(); // Then one alert should be removed from sbb-alert-group, tabindex should be set to 0, // focus should be on sbb-alert-group and accessibility title should still be rendered. - // Moreover, didDismissAlert event should have been fired. - await waitForCondition( - () => - didDismissAlertSpy.events.length === 1 && - element.querySelectorAll('sbb-alert').length === 1, - ); - expect(didDismissAlertSpy.count).to.be.equal(1); + await waitForCondition(() => element.querySelectorAll('sbb-alert').length === 1); expect(element.querySelectorAll('sbb-alert').length).to.be.equal(1); expect(element.tabIndex).to.be.equal(0); expect(document.activeElement!.id).to.be.equal(alertGroupId); expect( element.shadowRoot!.querySelector('.sbb-alert-group__title')!.textContent!.trim(), ).to.be.equal(accessibilityTitle); - expect(emptySpy.count).not.to.be.greaterThan(0); + expect(emptySpy.count).to.be.equal(0); // When clicking on close button of the second alert - element - .querySelector('sbb-alert')! + alert2 .shadowRoot!.querySelector( '.sbb-alert__close-button-wrapper sbb-transparent-button', )! .click(); await waitForLitRender(element); + await alert2ClosedEventSpy.calledOnce(); // Then the alert should be removed from sbb-alert-group, tabindex should be set to 0, // focus should be on sbb-alert-group, accessibility title should be removed and empty event should be fired. - await waitForCondition( - () => - didDismissAlertSpy.events.length === 2 && - element.querySelectorAll('sbb-alert').length === 0, - ); - expect(didDismissAlertSpy.count).to.be.equal(2); + await emptySpy.calledOnce(); expect(element.querySelectorAll('sbb-alert').length).to.be.equal(0); expect(element.tabIndex).to.be.equal(0); expect(document.activeElement!.id).to.be.equal(alertGroupId); expect(element.shadowRoot!.querySelector('.sbb-alert-group__title')).to.be.null; - expect(emptySpy.count).to.be.greaterThan(0); // When clicking away await sendMouse({ type: 'click', position: [0, 0] }); await waitForLitRender(element); // Then the active element id should be unset and tabindex should be removed - await waitForCondition(() => document.activeElement!.id === ''); expect(document.activeElement!.id).to.be.equal(''); expect(element.tabIndex).to.be.equal(-1); }); diff --git a/src/elements/alert/alert-group/alert-group.stories.ts b/src/elements/alert/alert-group/alert-group.stories.ts index b6a93f7f89..d7f4591347 100644 --- a/src/elements/alert/alert-group/alert-group.stories.ts +++ b/src/elements/alert/alert-group/alert-group.stories.ts @@ -78,7 +78,7 @@ const meta: Meta = { decorators: [withActions as Decorator], parameters: { actions: { - handles: [SbbAlertGroupElement.events.didDismissAlert, SbbAlertGroupElement.events.empty], + handles: [SbbAlertGroupElement.events.empty], }, docs: { extractComponentDescription: () => readme, diff --git a/src/elements/alert/alert-group/alert-group.ts b/src/elements/alert/alert-group/alert-group.ts index e41de61d88..6874a947f6 100644 --- a/src/elements/alert/alert-group/alert-group.ts +++ b/src/elements/alert/alert-group/alert-group.ts @@ -17,7 +17,6 @@ import style from './alert-group.scss?lit&inline'; * * @slot - Use the unnamed slot to add `sbb-alert` elements to the `sbb-alert-group`. * @slot accessibility-title - title for this `sbb-alert-group` which is only visible for screen reader users. - * @event {CustomEvent} didDismissAlert - Emits when an alert was removed from DOM. * @event {CustomEvent} empty - Emits when `sbb-alert-group` becomes empty. */ export @@ -25,7 +24,6 @@ export class SbbAlertGroupElement extends SbbHydrationMixin(LitElement) { public static override styles: CSSResultGroup = style; public static readonly events = { - didDismissAlert: 'didDismissAlert', empty: 'empty', } as const; @@ -50,12 +48,6 @@ class SbbAlertGroupElement extends SbbHydrationMixin(LitElement) { /** Whether the group currently has any alerts. */ @state() private accessor _hasAlerts: boolean = false; - /** Emits when an alert was removed from DOM. */ - private _didDismissAlert: EventEmitter = new EventEmitter( - this, - SbbAlertGroupElement.events.didDismissAlert, - ); - /** Emits when `sbb-alert-group` becomes empty. */ private _empty: EventEmitter = new EventEmitter(this, SbbAlertGroupElement.events.empty); @@ -64,13 +56,6 @@ class SbbAlertGroupElement extends SbbHydrationMixin(LitElement) { public override connectedCallback(): void { super.connectedCallback(); const signal = this._abort.signal; - this.addEventListener( - SbbAlertElement.events.dismissalRequested, - (e) => (e.target as SbbAlertElement).close(), - { - signal, - }, - ); this.addEventListener(SbbAlertElement.events.didClose, (e) => this._alertClosed(e), { signal, }); @@ -79,7 +64,6 @@ class SbbAlertGroupElement extends SbbHydrationMixin(LitElement) { private _alertClosed(event: Event): void { const target = event.target as SbbAlertElement; const hasFocusInsideAlertGroup = document.activeElement === target; - this._didDismissAlert.emit(target); // Restore focus if (hasFocusInsideAlertGroup) { diff --git a/src/elements/alert/alert-group/readme.md b/src/elements/alert/alert-group/readme.md index ab440beee0..20e3927962 100644 --- a/src/elements/alert/alert-group/readme.md +++ b/src/elements/alert/alert-group/readme.md @@ -1,4 +1,4 @@ -The `sbb-alert-group` manages the dismissal and accessibility of one or multiple +The `sbb-alert-group` manages the accessibility of one or multiple [sbb-alert](/docs/elements-sbb-alert-sbb-alert--docs) and also its visual gap between each other. ```html @@ -51,10 +51,9 @@ and therefore interrupts screen reader flow, to immediately read out the alert c ## Events -| Name | Type | Description | Inherited From | -| ----------------- | ------------------------------ | ------------------------------------------- | -------------- | -| `didDismissAlert` | `CustomEvent` | Emits when an alert was removed from DOM. | | -| `empty` | `CustomEvent` | Emits when `sbb-alert-group` becomes empty. | | +| Name | Type | Description | Inherited From | +| ------- | ------------------- | ------------------------------------------- | -------------- | +| `empty` | `CustomEvent` | Emits when `sbb-alert-group` becomes empty. | | ## Slots diff --git a/src/elements/alert/alert/alert.spec.ts b/src/elements/alert/alert/alert.spec.ts index 11d25b5470..e2af45215e 100644 --- a/src/elements/alert/alert/alert.spec.ts +++ b/src/elements/alert/alert/alert.spec.ts @@ -1,4 +1,4 @@ -import { assert, expect } from '@open-wc/testing'; +import { assert, aTimeout, expect } from '@open-wc/testing'; import { html } from 'lit/static-html.js'; import { fixture } from '../../core/testing/private.js'; @@ -19,7 +19,6 @@ describe(`sbb-alert`, () => { const didOpenSpy = new EventSpy(SbbAlertElement.events.didOpen); const willCloseSpy = new EventSpy(SbbAlertElement.events.willClose); const didCloseSpy = new EventSpy(SbbAlertElement.events.didClose); - const dismissalSpy = new EventSpy(SbbAlertElement.events.dismissalRequested); const alert: SbbAlertElement = await fixture( html`Interruption`, @@ -30,9 +29,6 @@ describe(`sbb-alert`, () => { await didOpenSpy.calledOnce(); expect(didOpenSpy.count).to.be.equal(1); - alert.requestDismissal(); - expect(dismissalSpy.count).to.be.equal(1); - alert.close(); await didCloseSpy.calledOnce(); @@ -40,6 +36,29 @@ describe(`sbb-alert`, () => { expect(didCloseSpy.count).to.be.equal(1); }); + it('should respect canceled willClose event', async () => { + const didOpenSpy = new EventSpy(SbbAlertElement.events.didOpen); + const willCloseSpy = new EventSpy(SbbAlertElement.events.willClose); + const didCloseSpy = new EventSpy(SbbAlertElement.events.didClose); + + const alert: SbbAlertElement = await fixture( + html`Interruption`, + ); + + alert.addEventListener(SbbAlertElement.events.willClose, (ev) => ev.preventDefault()); + + await didOpenSpy.calledOnce(); + + alert.close(); + + await willCloseSpy.calledOnce(); + expect(willCloseSpy.count).to.be.equal(1); + + // Wait a period to ensure the didCLose event was not dispatched. + await aTimeout(10); + expect(didCloseSpy.count).to.be.equal(0); + }); + it('should hide close button in readonly mode', async () => { alert = await fixture( html`Alert content`, diff --git a/src/elements/alert/alert/alert.stories.ts b/src/elements/alert/alert/alert.stories.ts index f3db653536..ea7b4431ac 100644 --- a/src/elements/alert/alert/alert.stories.ts +++ b/src/elements/alert/alert/alert.stories.ts @@ -1,8 +1,8 @@ import { withActions } from '@storybook/addon-actions/decorator'; import type { InputType } from '@storybook/types'; -import type { Meta, StoryObj, ArgTypes, Args, Decorator } from '@storybook/web-components'; +import type { Args, ArgTypes, Decorator, Meta, StoryObj } from '@storybook/web-components'; import type { TemplateResult } from 'lit'; -import { html, nothing } from 'lit'; +import { html } from 'lit'; import { sbbSpread } from '../../../storybook/helpers/spread.js'; @@ -10,11 +10,7 @@ import { SbbAlertElement } from './alert.js'; import readme from './readme.md?raw'; const Default = ({ 'content-slot-text': contentSlotText, ...args }: Args): TemplateResult => html` - (e.target! as SbbAlertElement).close()} - >${contentSlotText} + ${contentSlotText} `; const DefaultWithOtherContent = (args: Args): TemplateResult => { @@ -22,12 +18,6 @@ const DefaultWithOtherContent = (args: Args): TemplateResult => {
${Default(args)}

Other Content on the page.

- ${!args.readonly - ? html`

- Dismissal event of the alert has to be caught by the consumer and the alert has to be - manually removed from DOM. See 'sbb-alert-group' for demonstration. -

` - : nothing}
`; }; @@ -218,11 +208,7 @@ const meta: Meta = { decorators: [withActions as Decorator], parameters: { actions: { - handles: [ - SbbAlertElement.events.willOpen, - SbbAlertElement.events.didOpen, - SbbAlertElement.events.dismissalRequested, - ], + handles: [SbbAlertElement.events.willOpen, SbbAlertElement.events.didOpen], }, docs: { extractComponentDescription: () => readme, diff --git a/src/elements/alert/alert/alert.ts b/src/elements/alert/alert/alert.ts index 5c09c0757f..1b762dd76d 100644 --- a/src/elements/alert/alert/alert.ts +++ b/src/elements/alert/alert/alert.ts @@ -4,7 +4,6 @@ import { customElement, property } from 'lit/decorators.js'; import { type LinkTargetType, SbbOpenCloseBaseElement } from '../../core/base-elements.js'; import { SbbLanguageController } from '../../core/controllers.js'; import { forceType } from '../../core/decorators.js'; -import { EventEmitter } from '../../core/eventing.js'; import { i18nCloseAlert, i18nFindOutMore } from '../../core/i18n.js'; import { SbbIconNameMixin } from '../../icon.js'; import type { SbbTitleLevel } from '../../title.js'; @@ -26,7 +25,6 @@ import '../../title.js'; * @event {CustomEvent} didOpen - Emits when the opening animation ends. * @event {CustomEvent} willClose - Emits when the closing animation starts. Can be canceled. * @event {CustomEvent} didClose - Emits when the closing animation ends. - * @event {CustomEvent} dismissalRequested - Emits when dismissal of an alert was requested. */ export @customElement('sbb-alert') @@ -37,7 +35,6 @@ class SbbAlertElement extends SbbIconNameMixin(SbbOpenCloseBaseElement) { didOpen: 'didOpen', willClose: 'willClose', didClose: 'didClose', - dismissalRequested: 'dismissalRequested', } as const; /** @@ -96,24 +93,8 @@ class SbbAlertElement extends SbbIconNameMixin(SbbOpenCloseBaseElement) { /** The enabled animations. */ @property({ reflect: true }) public accessor animation: 'open' | 'close' | 'all' | 'none' = 'all'; - /** - * Emits when dismissal of an alert was requested. - * @deprecated - */ - private _dismissalRequested: EventEmitter = new EventEmitter( - this, - SbbAlertElement.events.dismissalRequested, - ); - private _language = new SbbLanguageController(this); - /** Requests dismissal of the alert. - * @deprecated in favour of 'willClose' and 'didClose' events - */ - public requestDismissal(): void { - this._dismissalRequested.emit(); - } - /** Open the alert. */ public open(): void { this.willOpen.emit(); @@ -122,7 +103,7 @@ class SbbAlertElement extends SbbIconNameMixin(SbbOpenCloseBaseElement) { /** Close the alert. */ public close(): void { - if (this.willClose.emit()) { + if (this.state === 'opened' && this.willClose.emit()) { this.state = 'closing'; } } @@ -194,7 +175,7 @@ class SbbAlertElement extends SbbIconNameMixin(SbbOpenCloseBaseElement) { negative size=${this.size === 'l' ? 'm' : this.size} icon-name="cross-small" - @click=${() => this.requestDismissal()} + @click=${() => this.close()} aria-label=${i18nCloseAlert[this._language.current]} class="sbb-alert__close-button" > diff --git a/src/elements/alert/alert/readme.md b/src/elements/alert/alert/readme.md index cdd26b061f..801d6d2f39 100644 --- a/src/elements/alert/alert/readme.md +++ b/src/elements/alert/alert/readme.md @@ -42,10 +42,6 @@ The `target` and `rel` properties are also configurable via the self-named prope ``` The `sbb-alert` can optionally be hidden by a user, if the `readonly` prop is not set. -Please note that clicking on the close button does not remove it from the DOM, this would be the responsibility -of the library consumer to do it by reacting to the specific event. -See also the [sbb-alert-group](/docs/elements-sbb-alert-sbb-alert-group--docs) -which automatically removes an alert after clicking the close button. ```html @@ -99,21 +95,19 @@ As a base rule, opening animations should be active if an alert arrives after th ## Methods -| Name | Privacy | Description | Parameters | Return | Inherited From | -| ------------------ | ------- | -------------------------------- | ---------- | ------ | ----------------------- | -| `close` | public | Close the alert. | | `void` | SbbOpenCloseBaseElement | -| `open` | public | Open the alert. | | `void` | SbbOpenCloseBaseElement | -| `requestDismissal` | public | Requests dismissal of the alert. | | `void` | | +| Name | Privacy | Description | Parameters | Return | Inherited From | +| ------- | ------- | ---------------- | ---------- | ------ | ----------------------- | +| `close` | public | Close the alert. | | `void` | SbbOpenCloseBaseElement | +| `open` | public | Open the alert. | | `void` | SbbOpenCloseBaseElement | ## Events -| Name | Type | Description | Inherited From | -| -------------------- | ------------------- | --------------------------------------------------------- | ----------------------- | -| `didClose` | `CustomEvent` | Emits when the closing animation ends. | SbbOpenCloseBaseElement | -| `didOpen` | `CustomEvent` | Emits when the opening animation ends. | SbbOpenCloseBaseElement | -| `dismissalRequested` | `CustomEvent` | Emits when dismissal of an alert was requested. | | -| `willClose` | `CustomEvent` | Emits when the closing animation starts. Can be canceled. | SbbOpenCloseBaseElement | -| `willOpen` | `CustomEvent` | Emits when the opening animation starts. | SbbOpenCloseBaseElement | +| Name | Type | Description | Inherited From | +| ----------- | ------------------- | --------------------------------------------------------- | ----------------------- | +| `didClose` | `CustomEvent` | Emits when the closing animation ends. | SbbOpenCloseBaseElement | +| `didOpen` | `CustomEvent` | Emits when the opening animation ends. | SbbOpenCloseBaseElement | +| `willClose` | `CustomEvent` | Emits when the closing animation starts. Can be canceled. | SbbOpenCloseBaseElement | +| `willOpen` | `CustomEvent` | Emits when the opening animation starts. | SbbOpenCloseBaseElement | ## Slots diff --git a/src/elements/notification/notification.ts b/src/elements/notification/notification.ts index 56d2960e18..51ded34f12 100644 --- a/src/elements/notification/notification.ts +++ b/src/elements/notification/notification.ts @@ -125,9 +125,8 @@ class SbbNotificationElement extends LitElement { } public close(): void { - if (this._state === 'opened') { + if (this._state === 'opened' && this._willClose.emit()) { this._state = 'closing'; - this._willClose.emit(); } } From ddde51b01e73158908b19b8798e8079b0be5f95c Mon Sep 17 00:00:00 2001 From: Jeremias Peier Date: Wed, 20 Nov 2024 11:11:15 +0100 Subject: [PATCH 2/2] fix: review --- .../alert/alert-group/alert-group.stories.ts | 11 ++++++--- .../alert-group/alert-group.visual.spec.ts | 23 ++++++++++++++----- src/elements/alert/alert/alert.stories.ts | 7 +++++- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/elements/alert/alert-group/alert-group.stories.ts b/src/elements/alert/alert-group/alert-group.stories.ts index d7f4591347..4796396f2f 100644 --- a/src/elements/alert/alert-group/alert-group.stories.ts +++ b/src/elements/alert/alert-group/alert-group.stories.ts @@ -5,12 +5,11 @@ import type { TemplateResult } from 'lit'; import { html } from 'lit'; import { sbbSpread } from '../../../storybook/helpers/spread.js'; +import { SbbAlertElement } from '../alert.js'; import { SbbAlertGroupElement } from './alert-group.js'; import readme from './readme.md?raw'; -import '../alert.js'; - const Template = (args: Args): TemplateResult => html` readme, diff --git a/src/elements/alert/alert-group/alert-group.visual.spec.ts b/src/elements/alert/alert-group/alert-group.visual.spec.ts index c3074e9580..a7ca29fee1 100644 --- a/src/elements/alert/alert-group/alert-group.visual.spec.ts +++ b/src/elements/alert/alert-group/alert-group.visual.spec.ts @@ -3,8 +3,9 @@ import { html } from 'lit/static-html.js'; import type { SbbTransparentButtonElement } from '../../button/transparent-button.js'; import { describeViewports, visualDiffDefault } from '../../core/testing/private.js'; +import { EventSpy, waitForCondition } from '../../core/testing.js'; +import { SbbAlertElement } from '../alert.js'; -import '../alert.js'; import './alert-group.js'; describe(`sbb-alert-group`, () => { @@ -32,12 +33,22 @@ describe(`sbb-alert-group`, () => { visualDiffDefault.with(async (setup) => { await setup.withFixture(html`${alert} ${alert}`); - const closeButton = setup.snapshotElement - .querySelector('sbb-alert')! - .shadowRoot!.querySelector('.sbb-alert__close-button')!; + setup.withPostSetupAction(async () => { + const alert = setup.snapshotElement.querySelector('sbb-alert')!; + const didCloseEventSpy = new EventSpy(SbbAlertElement.events.didClose, alert); - closeButton.focus(); - await sendKeys({ press: 'Enter' }); + // As registering an eventSpy is too late we have to use waitForCondition(). + await waitForCondition(() => alert.getAttribute('data-state') === 'opened'); + + const closeButton = setup.snapshotElement + .querySelector('sbb-alert')! + .shadowRoot!.querySelector('.sbb-alert__close-button')!; + + closeButton.focus(); + await sendKeys({ press: 'Enter' }); + + await didCloseEventSpy.calledOnce(); + }); }), ); }); diff --git a/src/elements/alert/alert/alert.stories.ts b/src/elements/alert/alert/alert.stories.ts index ea7b4431ac..2c7b0892a5 100644 --- a/src/elements/alert/alert/alert.stories.ts +++ b/src/elements/alert/alert/alert.stories.ts @@ -208,7 +208,12 @@ const meta: Meta = { decorators: [withActions as Decorator], parameters: { actions: { - handles: [SbbAlertElement.events.willOpen, SbbAlertElement.events.didOpen], + handles: [ + SbbAlertElement.events.willOpen, + SbbAlertElement.events.didOpen, + SbbAlertElement.events.willClose, + SbbAlertElement.events.didClose, + ], }, docs: { extractComponentDescription: () => readme,