Skip to content

Normalize _value to array in kol-select to handle single values #7760

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.

Sign up for GitHub

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

Closed
donchi-donald wants to merge 9 commits into from
Closed

Normalize _value to array in kol-select to handle single values #7760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4155eb2
fix(kol-select): support single value for _value prop by normalizing …
donchi-donald May 22, 2025
cd84fb2
fix(kol-select): normalize _value to array if string or undefined (#7…
donchi-donald May 22, 2025
a8ad13f
fix(kol-select): add snapshot.spec.tsx.snap (#7758)
donchi-donald May 22, 2025
7602a92
chore(select): clean up example by removing hardcoded _value (#7758)
donchi-donald May 30, 2025
8059992
docs(select): simplify _value examples using string instead of array …
donchi-donald May 30, 2025
c008d7d
Merge branch 'develop' into 7758-allow-single-value-in-kolselect
donchi-donald Jun 4, 2025
215fd0e
fix(kol-select): update playwright test (#7758)
donchi-donald Jun 4, 2025
3ef5be5
fix(kol-select): Fix incorrect usage and placement of 'select' in cod…
donchi-donald Jun 11, 2025
9fa62ec
fix(pagination): ensure page size change triggers correct event and v…
donchi-donald Jun 11, 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
4 changes: 2 additions & 2 deletions packages/components/src/components/pagination/shadow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ export class KolPagination implements PaginationAPI {
_on={{
onChange: this.onChangePageSize,
}}
_value={[this.state._pageSize]}
_value={this.state._pageSize}
></KolSelectTag>
)}
</Host>
Expand Down Expand Up @@ -280,7 +280,7 @@ export class KolPagination implements PaginationAPI {
};

private onChangePageSize = (event: Event, value: unknown) => {
value = parseInt((value as string[])[0]);
value = parseInt(value as string);
if (typeof value === 'number' && value > 0 && this._pageSize !== value) {
this._pageSize = value;
const timeout = setTimeout(() => {
Expand Down
47 changes: 45 additions & 2 deletions packages/components/src/components/select/controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ export class SelectController extends InputIconController implements SelectWatch
};

private readonly beforePatchOptions = (_value: unknown, nextState: Map<string, unknown>): void => {
const raw = nextState.get('_value');
if (raw !== undefined && !Array.isArray(raw)) {
nextState.set('_value', [raw]);
}
const options = nextState.has('_options') ? nextState.get('_options') : this.component.state._options;
if (Array.isArray(options) && options.length > 0) {
this.keyOptionMap.clear();
Expand Down Expand Up @@ -83,6 +87,7 @@ export class SelectController extends InputIconController implements SelectWatch
}

public validateMultiple(value?: boolean): void {
this.assertComponentValueMatchesMultiplicity(value === true);
watchBoolean(this.component, '_multiple', value, {
hooks: {
afterPatch: this.afterPatchOptions,
Expand All @@ -105,8 +110,9 @@ export class SelectController extends InputIconController implements SelectWatch
validateRows(this.component, value);
}

public validateValue(value?: Stringified<StencilUnknown[]>): void {
watchJsonArrayString(this.component, '_value', () => true, value, undefined, {
public validateValue(value?: Stringified<StencilUnknown[]> | Stringified<StencilUnknown>): void {
this.assertValueMatchesMultiplicity(value);
watchJsonArrayString(this.component, '_value', () => true, value === undefined ? [] : Array.isArray(value) ? value : [value], undefined, {
hooks: {
afterPatch: this.afterPatchOptions,
beforePatch: this.beforePatchOptions,
Expand All @@ -122,4 +128,41 @@ export class SelectController extends InputIconController implements SelectWatch
this.validateRows(this.component._rows);
this.validateValue(this.component._value);
}

private assertValueMatchesMultiplicity(value?: Stringified<StencilUnknown[]> | Stringified<StencilUnknown>): void {
const isArray = Array.isArray(value);
const isMultiple = this.component._multiple === true;

if (isMultiple) {
if (value !== undefined && !isArray) {
throw new Error(
`↑ The schema for the property (_value) is not valid for multiple mode. Expected an array. The value will not be changed. (received = ${JSON.stringify(value)})`,
);
}
} else {
if (isArray) {
throw new Error(
`↑ The schema for the property (_value) is not valid for single mode. Expected a single value. The value will not be changed. (received = ${JSON.stringify(value)})`,
);
}
}
}

private assertComponentValueMatchesMultiplicity(isMultiple: boolean): void {
const rawValue = this.component._value;

if (isMultiple) {
if (rawValue !== undefined && !Array.isArray(rawValue)) {
throw new Error(
`↑ The schema for the property (_value) is not valid for multiple mode. Expected an array. The value will not be changed. (current = ${JSON.stringify(rawValue)})`,
);
}
} else {
if (Array.isArray(rawValue)) {
throw new Error(
`↑ The schema for the property (_value) is not valid for single mode. Expected a single value. The value will not be changed. (current = ${JSON.stringify(rawValue)})`,
);
}
}
}
}
2 changes: 1 addition & 1 deletion packages/components/src/components/select/select.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type { FillAction } from '../../e2e/utils/FillAction';
import type { Page } from '@playwright/test';

const COMPONENT_NAME = 'kol-select';
const TEST_VALUE = ['E'];
const TEST_VALUE = 'E';
const TEST_LABEL = 'East';
const OPTIONS = [
{ label: 'North', value: 'N' },
Expand Down
31 changes: 23 additions & 8 deletions packages/components/src/components/select/shadow.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,12 @@ export class KolSelect implements SelectAPI, FocusableElement {

@Method()
// eslint-disable-next-line @typescript-eslint/require-await
public async getValue(): Promise<StencilUnknown[]> {
return this.state._value;
public async getValue(): Promise<StencilUnknown[] | StencilUnknown> {
if (this._multiple) {
Copy link
Copy Markdown
Contributor

@sdvg sdvg Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_multiple_ bringt hier einen interessanten Aspekt hinein, Gedanken dazu:

1. Hintergrund zur _multiple-Prop

  • Die _multiple-Prop wird aktuell einzig dazu verwendet, das native <select>-Element mit multiple auszustatten.
  • Es gab bislang keine enge Verknüpfung zwischen _multiple und dem Datentyp von _value (Single-Value vs. Array-Value).

2. Typ-Ermittlung von _value

  • Statt sich auf den Typ von _multiple zu verlassen, sollte man besser den initialen Wert prüfen (siehe kol-input-number, kol-input-date).

3. Rückwärtskompatibilität

  • Für bestehende Implementierungen sollten beide Varianten weiterhin funktionieren:
    1. Single-Value:
      <KolSelect _value="single value" />
    2. Array-Value:
      <KolSelect _value={["array value"]} />
  • Die Prop _multiple darf dabei nicht implizit den Rückgabewert verändern, sondern sollte nur das Attribut an das native <select> weiterreichen.

4. Problemfall: Kombination von _value="single value" und _multiple

<KolSelect _value="single value" _multiple />

  • Hier ist unklar, ob der "single value" oder ein Array zurückgegeben werden soll.
  • In diesem Fall macht es m.E. keinen Sinn, einen einfachen String als Rückgabewert zu haben, wenn _multiple gesetzt ist.

5. Mögliche Lösungsansätze

  1. Fehler/Warnung ausgeben

    • Wenn _multiple={true} und _value ist kein Array, würde eine Runtime-Exception oder zumindest eine Warnung im Log erscheinen.

  2. Nur noch konsistente Kombis unterstützen (Breaking Change)

    • _multiple={false} → nur Single-Value
    • _multiple={true} → nur Array-Value
    • Vorteil: Typensicherheit und klare Erwartung, Nachteil: möglicher Bruch mit existierendem Code.

Fazit/Empfehlung

  • Meiner Meinung nach sollten wir kurzfristig zumindest eine Warnung ausgeben, wenn jemand _multiple mit einem Single-Value nutzt.
  • Langfristig könnte man überlegen, auf eine strengere Typisierung umzusteigen (Option 2), sobald ein Major-Release ansteht.

@deleonio @laske185 Wollt ihr hierzu noch Input geben?

Copy link
Copy Markdown
Contributor Author

@donchi-donald donchi-donald Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vielen Dank für das ausführliche Feedback – das hilft mir, den Zusammenhang zwischen _multiple und _value jetzt deutlich besser zu verstehen. Ich sehe ein, warum eine klare Trennung bzw. konsistente Handhabung hier wichtig ist.

Ich werde die entsprechenden Anpassungen sofort vornehmen und das Verhalten entsprechend korrigieren, sodass bei Nutzung von _multiple ein Array erwartet wird bzw. zumindest eine Warnung erscheint, wenn dies nicht der Fall ist.

Danke nochmal für die Erklärung!

Copy link
Copy Markdown
Contributor

@sdvg sdvg Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @donchi-donald

Wir haben das Thema heute morgen noch einmal im Team besprochen:

Der Konsens ist jetzt, dass doch die _multiple-Property berücksichtigt, und sich auf den (erwarteten) _value auswirken soll. Wir wollen also praktisch den oben beschriebenen Lösungsansatz 2 umsetzen.

Das heißt auch, dass wir die Validierung entsprechend anpassen müssen:
-> Ändern sich _value oder _multiple, muss der _value darauf validiert werden, dass er zur _multiple-Property passt.

  • _value={["multi"]} _multiple ✅ valid
  • _value={"single"} _multiple ❌ invalid
  • _value={"single"} _multiple={false} ✅ valid
  • _value={"single"} _multiple ❌ invalid

Danke für deine Geduld mit dem hin-und-her, das Ticket hätte initial besser durchdacht sein können.

Copy link
Copy Markdown
Contributor Author

@donchi-donald donchi-donald Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sdvg ,

vielen Dank für das Update und die Klarstellung.

Tatsächlich hatte ich die Lösung bereits unter Berücksichtigung des initialen Wertes implementiert. Ich verstehe nun aber, dass zusätzlich auch die _multiple-Property aktiv berücksichtigt und bei Änderungen entsprechend validiert werden soll.

Ich werde meinen Code zeitnah aktualisieren, um diese Validierungslogik mit einzubeziehen: Wenn sich _value oder _multiple ändern, wird _value so validiert, dass es konsistent zur _multiple-Property passt.

Vielen Dank nochmals für das konstruktive Feedback und die klare Richtung!
Ich passe die Lösung entsprechend an.

Beste Grüße

return this.state._value;
} else {
return Array.isArray(this.state._value) && this.state._value.length > 0 ? this.state._value[0] : this.state._value;
}
}

@Method()
Expand Down Expand Up @@ -223,7 +227,7 @@ export class KolSelect implements SelectAPI, FocusableElement {
/**
* Defines the value of the input.
*/
@Prop({ mutable: true, reflect: true }) public _value?: Stringified<StencilUnknown[]>;
@Prop({ mutable: true, reflect: true }) public _value?: Stringified<StencilUnknown[]> | Stringified<StencilUnknown>;

@State() public state: SelectStates = {
_hasValue: false,
Expand Down Expand Up @@ -341,7 +345,7 @@ export class KolSelect implements SelectAPI, FocusableElement {
}

@Watch('_value')
public validateValue(value?: Stringified<StencilUnknown[]>): void {
public validateValue(value?: Stringified<StencilUnknown[]> | Stringified<StencilUnknown>): void {
this.controller.validateValue(value);
}

Expand All @@ -354,14 +358,25 @@ export class KolSelect implements SelectAPI, FocusableElement {
}

private onInput(event: Event): void {
this._value = Array.from(this.selectRef?.options || [])
.filter((option) => option.selected === true)
const selectedValues = Array.from(this.selectRef?.options || [])
.filter((option) => option.selected)
.map((option) => this.controller.getOptionByKey(option.value)?.value as string);

this.controller.onFacade.onInput(event, true, this._value);
if (this._multiple) {
this._value = selectedValues;
this.controller.onFacade.onInput(event, true, selectedValues);
} else {
const singleValue: StencilUnknown = selectedValues.length > 0 ? selectedValues[0] : undefined;
this._value = singleValue;
this.controller.onFacade.onInput(event, true, singleValue);
}
}

private onChange(event: Event): void {
this.controller.onFacade.onChange(event, this._value);
if (this._multiple) {
this.controller.onFacade.onChange(event, this._value as StencilUnknown[]);
} else {
this.controller.onFacade.onChange(event, this._value as StencilUnknown);
}
}
}
Loading
⚡