From c60dd1c56b6f419e525fd3ecdebc488a5d4aee6d Mon Sep 17 00:00:00 2001 From: Lukas Spirig Date: Tue, 23 Jan 2024 14:20:37 +0100 Subject: [PATCH] refactor: review --- src/components/autocomplete/autocomplete.ts | 1 + .../breadcrumb-group.spec.snap.js | 6 +- .../breadcrumb-group/breadcrumb-group.e2e.ts | 4 +- .../breadcrumb-group/breadcrumb-group.spec.ts | 6 +- .../breadcrumb-group/breadcrumb-group.ts | 5 +- .../named-slot-list-element.ts | 9 +-- .../common-behaviors/slot-child-observer.ts | 4 +- .../__snapshots__/link-list.spec.snap.js | 72 +++++++++---------- src/components/link-list/link-list.ts | 1 + .../menu/menu/__snapshots__/menu.spec.snap.js | 8 +-- src/components/menu/menu/menu.spec.ts | 8 +-- .../navigation-list.spec.snap.js | 14 ++-- .../navigation-list/navigation-list.spec.ts | 10 +-- .../navigation-list/navigation-list.ts | 15 ++-- .../navigation-marker/navigation-marker.ts | 1 + src/components/option/optgroup/optgroup.ts | 1 + .../__snapshots__/skiplink-list.spec.snap.js | 18 ++--- .../skiplink-list/skiplink-list.spec.ts | 16 ++--- src/components/skiplink-list/skiplink-list.ts | 19 +++-- .../tag/tag-group/tag-group.spec.ts | 16 ++--- src/components/tag/tag-group/tag-group.ts | 1 + .../train-formation.spec.snap.js | 6 +- .../train-formation/train-formation.spec.ts | 6 +- .../train/train-formation/train-formation.ts | 1 + .../train/train-wagon/train-wagon.e2e.ts | 4 +- .../train/train-wagon/train-wagon.spec.ts | 12 ++-- src/components/train/train/train.ts | 1 + 27 files changed, 142 insertions(+), 123 deletions(-) diff --git a/src/components/autocomplete/autocomplete.ts b/src/components/autocomplete/autocomplete.ts index d8a18b3b24..29a48152f2 100644 --- a/src/components/autocomplete/autocomplete.ts +++ b/src/components/autocomplete/autocomplete.ts @@ -239,6 +239,7 @@ export class SbbAutocompleteElement extends SlotChildObserver(LitElement) { } protected override willUpdate(changedProperties: PropertyValues): void { + super.willUpdate(changedProperties); if (changedProperties.has('origin')) { this._resetOriginClickListener(this.origin, changedProperties.get('origin')); } diff --git a/src/components/breadcrumb/breadcrumb-group/__snapshots__/breadcrumb-group.spec.snap.js b/src/components/breadcrumb/breadcrumb-group/__snapshots__/breadcrumb-group.spec.snap.js index 233319709f..9892581766 100644 --- a/src/components/breadcrumb/breadcrumb-group/__snapshots__/breadcrumb-group.spec.snap.js +++ b/src/components/breadcrumb/breadcrumb-group/__snapshots__/breadcrumb-group.spec.snap.js @@ -4,7 +4,7 @@ export const snapshots = {}; snapshots["sbb-breadcrumb-group renders"] = `
  1. - +
  2. - +
  3. - +
diff --git a/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.e2e.ts b/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.e2e.ts index 4926f67a00..2dc512f10c 100644 --- a/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.e2e.ts +++ b/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.e2e.ts @@ -91,8 +91,8 @@ describe('sbb-breadcrumb-group', () => { // only two slots are displayed, and the second is the last one const slots = breadcrumbGroup.shadowRoot!.querySelectorAll('li > slot'); expect(slots.length).to.be.equal(2); - expect(slots[0]).to.have.attribute('name', 'child-0'); - expect(slots[1]).to.have.attribute('name', 'child-6'); + expect(slots[0]).to.have.attribute('name', 'li-0'); + expect(slots[1]).to.have.attribute('name', 'li-6'); }); it('keyboard navigation with ellipsis', async () => { diff --git a/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.spec.ts b/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.spec.ts index 0414d47cd9..d8d2b5c4f8 100644 --- a/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.spec.ts +++ b/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.spec.ts @@ -19,11 +19,11 @@ describe('sbb-breadcrumb-group', () => { expect(root).dom.to.be.equal(` - - + + One - + Two diff --git a/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.ts b/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.ts index 9ef71b86ec..392459b6ba 100644 --- a/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.ts +++ b/src/components/breadcrumb/breadcrumb-group/breadcrumb-group.ts @@ -67,6 +67,7 @@ export class SbbBreadcrumbGroupElement extends NamedSlotListElement>): void { + super.willUpdate(changedProperties); if (changedProperties.has('listChildren')) { this._syncBreadcrumbs(); } @@ -143,7 +144,7 @@ export class SbbBreadcrumbGroupElement extends NamedSlotListElement - +
  • - +
  • `; } diff --git a/src/components/core/common-behaviors/named-slot-list-element.ts b/src/components/core/common-behaviors/named-slot-list-element.ts index e729071f8a..f3fc3541fe 100644 --- a/src/components/core/common-behaviors/named-slot-list-element.ts +++ b/src/components/core/common-behaviors/named-slot-list-element.ts @@ -5,7 +5,7 @@ import { state } from 'lit/decorators.js'; import { SlotChildObserver } from './slot-child-observer'; const SSR_CHILD_COUNT_ATTRIBUTE = 'data-ssr-child-count'; -const SLOTNAME_PREFIX = 'child'; +const SLOTNAME_PREFIX = 'li'; /** * Helper type for willUpdate or similar checks. @@ -58,17 +58,12 @@ export abstract class NamedSlotListElement< .filter((c) => !listChildren.includes(c)) .forEach((c) => c.removeAttribute('slot')); this.listChildren = listChildren; - this.listChildren.forEach((child, index) => { - child.setAttribute('slot', `${SLOTNAME_PREFIX}-${index}`); - this.formatChild?.(child); - }); + this.listChildren.forEach((c, index) => c.setAttribute('slot', `${SLOTNAME_PREFIX}-${index}`)); // Remove the ssr attribute, once we have actually initialized the children elements. this.removeAttribute(SSR_CHILD_COUNT_ATTRIBUTE); } - protected formatChild?(child: C): void; - /** * Renders list and list slots for slotted children or an amount of list slots * corresponding to the `data-ssr-child-count` attribute value. diff --git a/src/components/core/common-behaviors/slot-child-observer.ts b/src/components/core/common-behaviors/slot-child-observer.ts index 98a21a8fd5..2cff8635c7 100644 --- a/src/components/core/common-behaviors/slot-child-observer.ts +++ b/src/components/core/common-behaviors/slot-child-observer.ts @@ -41,7 +41,7 @@ export const SlotChildObserver = >( protected override createRenderRoot(): HTMLElement | DocumentFragment { // Check whether hydration is needed by checking whether the shadow root // is available before createRenderRoot is called. - this._needsHydration = !!this.shadowRoot; + this._needsHydration = !!this.shadowRoot && 'litElementHydrateSupport' in globalThis; return super.createRenderRoot(); } @@ -49,7 +49,7 @@ export const SlotChildObserver = >( super.willUpdate(changedProperties); // If hydration is not needed we can immediately check for children // in the first update. - if (!this._needsHydration && !this.hasUpdated) { + if (!this.hasUpdated && !this._needsHydration) { this.checkChildren(); } } diff --git a/src/components/link-list/__snapshots__/link-list.spec.snap.js b/src/components/link-list/__snapshots__/link-list.spec.snap.js index 87e8bb1c12..4614aa59f2 100644 --- a/src/components/link-list/__snapshots__/link-list.spec.snap.js +++ b/src/components/link-list/__snapshots__/link-list.spec.snap.js @@ -16,15 +16,15 @@ snapshots["sbb-link-list should render named slots if data-ssr-child-count attri @@ -38,7 +38,7 @@ snapshots["sbb-link-list should render named slots if data-ssr-child-count attri snapshots["sbb-link-list rendered with a slotted title in light DOM"] = ` @@ -64,7 +64,7 @@ snapshots["sbb-link-list rendered with a slotted title in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-1" + slot="li-1" tabindex="0" variant="block" > @@ -76,7 +76,7 @@ snapshots["sbb-link-list rendered with a slotted title in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-2" + slot="li-2" tabindex="0" variant="block" > @@ -88,7 +88,7 @@ snapshots["sbb-link-list rendered with a slotted title in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-3" + slot="li-3" tabindex="0" variant="block" > @@ -100,7 +100,7 @@ snapshots["sbb-link-list rendered with a slotted title in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-4" + slot="li-4" tabindex="0" variant="block" > @@ -128,23 +128,23 @@ snapshots["sbb-link-list rendered with a slotted title in shadow DOM"] = class="sbb-link-list" >
  • - +
  • - +
  • - +
  • - +
  • - +
  • @@ -158,7 +158,7 @@ snapshots["sbb-link-list rendered with a slotted title in shadow DOM"] = snapshots["sbb-link-list rendered with a title from properties in light DOM"] = ` @@ -182,7 +182,7 @@ snapshots["sbb-link-list rendered with a title from properties in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-1" + slot="li-1" tabindex="0" variant="block" > @@ -194,7 +194,7 @@ snapshots["sbb-link-list rendered with a title from properties in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-2" + slot="li-2" tabindex="0" variant="block" > @@ -206,7 +206,7 @@ snapshots["sbb-link-list rendered with a title from properties in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-3" + slot="li-3" tabindex="0" variant="block" > @@ -218,7 +218,7 @@ snapshots["sbb-link-list rendered with a title from properties in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-4" + slot="li-4" tabindex="0" variant="block" > @@ -247,23 +247,23 @@ snapshots["sbb-link-list rendered with a title from properties in shadow DOM"] = class="sbb-link-list" >
  • - +
  • - +
  • - +
  • - +
  • - +
  • @@ -277,7 +277,7 @@ snapshots["sbb-link-list rendered with a title from properties in shadow DOM"] = snapshots["sbb-link-list rendered without a title in light DOM"] = ` @@ -287,7 +287,7 @@ snapshots["sbb-link-list rendered without a title in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-0" + slot="li-0" tabindex="0" variant="block" > @@ -299,7 +299,7 @@ snapshots["sbb-link-list rendered without a title in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-1" + slot="li-1" tabindex="0" variant="block" > @@ -311,7 +311,7 @@ snapshots["sbb-link-list rendered without a title in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-2" + slot="li-2" tabindex="0" variant="block" > @@ -323,7 +323,7 @@ snapshots["sbb-link-list rendered without a title in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-3" + slot="li-3" tabindex="0" variant="block" > @@ -335,7 +335,7 @@ snapshots["sbb-link-list rendered without a title in light DOM"] = href="https://www.sbb.ch/de/hilfe-und-kontakt/erstattung-entschaedigung/rueckerstattung-von-billetten.html" role="link" size="s" - slot="child-4" + slot="li-4" tabindex="0" variant="block" > @@ -360,23 +360,23 @@ snapshots["sbb-link-list rendered without a title in shadow DOM"] = diff --git a/src/components/link-list/link-list.ts b/src/components/link-list/link-list.ts index a3e02a8197..063ece694b 100644 --- a/src/components/link-list/link-list.ts +++ b/src/components/link-list/link-list.ts @@ -51,6 +51,7 @@ export class SbbLinkListElement extends NamedSlotListElement { private _namedSlots = new NamedSlotStateController(this); protected override willUpdate(changedProperties: PropertyValues>): void { + super.willUpdate(changedProperties); if ( changedProperties.has('size') || changedProperties.has('negative') || diff --git a/src/components/menu/menu/__snapshots__/menu.spec.snap.js b/src/components/menu/menu/__snapshots__/menu.spec.snap.js index 1fc19a9677..a469fdc83f 100644 --- a/src/components/menu/menu/__snapshots__/menu.spec.snap.js +++ b/src/components/menu/menu/__snapshots__/menu.spec.snap.js @@ -60,19 +60,19 @@ snapshots["sbb-menu renders with list"] =
    • - +
    • - +
    • - +
    • - +
    diff --git a/src/components/menu/menu/menu.spec.ts b/src/components/menu/menu/menu.spec.ts index 5b43c525c6..0bbb7786c3 100644 --- a/src/components/menu/menu/menu.spec.ts +++ b/src/components/menu/menu/menu.spec.ts @@ -51,16 +51,16 @@ describe('sbb-menu', () => { ` - + View - + Edit - + Details - + Cancel diff --git a/src/components/navigation/navigation-list/__snapshots__/navigation-list.spec.snap.js b/src/components/navigation/navigation-list/__snapshots__/navigation-list.spec.snap.js index 7f1ce82aaf..297d3f69f2 100644 --- a/src/components/navigation/navigation-list/__snapshots__/navigation-list.spec.snap.js +++ b/src/components/navigation/navigation-list/__snapshots__/navigation-list.spec.snap.js @@ -14,15 +14,15 @@ snapshots["sbb-navigation-list should render named slots if data-ssr-child-count class="sbb-navigation-list__content" >
  • - +
  • - +
  • - +
  • @@ -46,19 +46,19 @@ snapshots["sbb-navigation-list renders"] = class="sbb-navigation-list__content" >
  • - +
  • - +
  • - +
  • - +
  • diff --git a/src/components/navigation/navigation-list/navigation-list.spec.ts b/src/components/navigation/navigation-list/navigation-list.spec.ts index 418a1055c3..dc7e24e26a 100644 --- a/src/components/navigation/navigation-list/navigation-list.spec.ts +++ b/src/components/navigation/navigation-list/navigation-list.spec.ts @@ -15,17 +15,17 @@ describe('sbb-navigation-list', () => { expect(root).dom.to.be.equal( ` - - + + Tickets & Offers - + Vacations & Recreation - + Travel information - + Help & Contact diff --git a/src/components/navigation/navigation-list/navigation-list.ts b/src/components/navigation/navigation-list/navigation-list.ts index d592a530b8..b4614b4ceb 100644 --- a/src/components/navigation/navigation-list/navigation-list.ts +++ b/src/components/navigation/navigation-list/navigation-list.ts @@ -1,8 +1,12 @@ -import type { CSSResultGroup, TemplateResult } from 'lit'; +import type { CSSResultGroup, PropertyValues, TemplateResult } from 'lit'; import { html } from 'lit'; import { customElement, property } from 'lit/decorators.js'; -import { NamedSlotListElement, NamedSlotStateController } from '../../core/common-behaviors'; +import { + NamedSlotListElement, + NamedSlotStateController, + type WithListChildren, +} from '../../core/common-behaviors'; import type { SbbNavigationActionElement } from '../navigation-action'; import style from './navigation-list.scss?lit&inline'; @@ -28,8 +32,11 @@ export class SbbNavigationListElement extends NamedSlotListElement>): void { + super.willUpdate(changedProperties); + if (changedProperties.has('listChildren')) { + this.listChildren.forEach((c) => (c.size = 'm')); + } } protected override render(): TemplateResult { diff --git a/src/components/navigation/navigation-marker/navigation-marker.ts b/src/components/navigation/navigation-marker/navigation-marker.ts index 2aaf20f83d..559815279d 100644 --- a/src/components/navigation/navigation-marker/navigation-marker.ts +++ b/src/components/navigation/navigation-marker/navigation-marker.ts @@ -30,6 +30,7 @@ export class SbbNavigationMarkerElement extends NamedSlotListElement>): void { + super.willUpdate(changedProperties); setAttribute(this, 'data-has-active-action', !!this._currentActiveAction); if (changedProperties.has('size') || changedProperties.has('listChildren')) { this._updateMarkerActions(); diff --git a/src/components/option/optgroup/optgroup.ts b/src/components/option/optgroup/optgroup.ts index ffd48d55e3..7cfa2f7ee3 100644 --- a/src/components/option/optgroup/optgroup.ts +++ b/src/components/option/optgroup/optgroup.ts @@ -64,6 +64,7 @@ export class SbbOptGroupElement extends SlotChildObserver(LitElement) { } protected override willUpdate(changedProperties: PropertyValues): void { + super.willUpdate(changedProperties); if (changedProperties.has('disabled')) { this._proxyDisabledToOptions(); } diff --git a/src/components/skiplink-list/__snapshots__/skiplink-list.spec.snap.js b/src/components/skiplink-list/__snapshots__/skiplink-list.spec.snap.js index d259bed3a6..fff23acc73 100644 --- a/src/components/skiplink-list/__snapshots__/skiplink-list.spec.snap.js +++ b/src/components/skiplink-list/__snapshots__/skiplink-list.spec.snap.js @@ -21,15 +21,15 @@ snapshots["sbb-skiplink-list should render named slots if data-ssr-child-count a class="sbb-skiplink-list" >
  • - +
  • - +
  • - +
  • @@ -61,15 +61,15 @@ snapshots["sbb-skiplink-list renders"] = class="sbb-skiplink-list" >
  • - +
  • - +
  • - +
  • @@ -102,15 +102,15 @@ snapshots["sbb-skiplink-list renders with title"] = class="sbb-skiplink-list" >
  • - +
  • - +
  • - +
  • diff --git a/src/components/skiplink-list/skiplink-list.spec.ts b/src/components/skiplink-list/skiplink-list.spec.ts index d92e45f264..8a813bcb06 100644 --- a/src/components/skiplink-list/skiplink-list.spec.ts +++ b/src/components/skiplink-list/skiplink-list.spec.ts @@ -16,10 +16,10 @@ describe('sbb-skiplink-list', () => { expect(root).dom.to.be.equal( ` - - Link 1 - Link 2 - Link 3 + + Link 1 + Link 2 + Link 3 `, ); @@ -37,10 +37,10 @@ describe('sbb-skiplink-list', () => { expect(root).dom.to.be.equal( ` - - Link 1 - Link 2 - Link 3 + + Link 1 + Link 2 + Link 3 `, ); diff --git a/src/components/skiplink-list/skiplink-list.ts b/src/components/skiplink-list/skiplink-list.ts index 73df58f992..9cd8f30928 100644 --- a/src/components/skiplink-list/skiplink-list.ts +++ b/src/components/skiplink-list/skiplink-list.ts @@ -1,8 +1,12 @@ -import type { CSSResultGroup, TemplateResult } from 'lit'; +import type { CSSResultGroup, PropertyValues, TemplateResult } from 'lit'; import { html, nothing } from 'lit'; import { customElement, property } from 'lit/decorators.js'; -import { NamedSlotListElement, NamedSlotStateController } from '../core/common-behaviors'; +import { + NamedSlotListElement, + NamedSlotStateController, + type WithListChildren, +} from '../core/common-behaviors'; import type { SbbLinkElement } from '../link'; import type { TitleLevel } from '../title'; @@ -31,9 +35,14 @@ export class SbbSkiplinkListElement extends NamedSlotListElement new NamedSlotStateController(this); } - protected override formatChild(child: SbbLinkElement): void { - child.size = 'm'; - child.negative = true; + protected override willUpdate(changedProperties: PropertyValues>): void { + super.willUpdate(changedProperties); + if (changedProperties.has('listChildren')) { + for (const child of this.listChildren) { + child.size = 'm'; + child.negative = true; + } + } } protected override render(): TemplateResult { diff --git a/src/components/tag/tag-group/tag-group.spec.ts b/src/components/tag/tag-group/tag-group.spec.ts index 5caf387e54..42171dba5b 100644 --- a/src/components/tag/tag-group/tag-group.spec.ts +++ b/src/components/tag/tag-group/tag-group.spec.ts @@ -16,14 +16,14 @@ describe('sbb-tag-group', () => { expect(root).dom.to.be.equal( ` - + First tag - + Second tag -
    - +
    + Third tag
    @@ -34,16 +34,16 @@ describe('sbb-tag-group', () => {
    • - +
    • - +
    • - +
    • - +