Merge branch 'opt-group_fix'

CHANGELOG.md

@@ -12,6 +12,8 @@
 * Fix regression "no choices to choose from"/"no results found" notice did not reliably trigger. [#1185](https://github.com/Choices-js/Choices/issues/1185) [#1191](https://github.com/Choices-js/Choices/issues/1191)
 * Fix regression of `UnhighlightItem` event not firing [#1173](https://github.com/Choices-js/Choices/issues/1173)
 * Fix `clearChoices()` would remove items, and clear the search flag.
+* Fixes for opt-group handling/rendering
+* Fix `removeChoice()` did not properly remove a choice which was part of a group
 
 ### Chore
 * Add e2e tests for "no choices" behavior to match v10

public/test/select-multiple/index.html

@@ -914,7 +914,9 @@ <h2>Select multiple inputs</h2>
             multiple
           >
             <option value="Choice 1">Choice 1</option>
-            <option value="Choice 2">Choice 2</option>
+            <optgroup label="test">
+              <option value="Choice 2">Choice 2</option>
+            </optgroup>
             <option value="Choice 3">Choice 3</option>
           </select>
           <button class="destroy">Destroy</button>

public/test/select-one/index.html

@@ -870,7 +870,9 @@ <h2>Select one inputs</h2>
             id="choices-new-destroy-init"
           >
             <option value="Choice 1">Choice 1</option>
-            <option value="Choice 2">Choice 2</option>
+            <optgroup label="test">
+              <option value="Choice 2">Choice 2</option>
+            </optgroup>
             <option value="Choice 3">Choice 3</option>
           </select>
           <button class="destroy">Destroy</button>
@@ -926,7 +928,7 @@ <h2>Select one inputs</h2>
           </script>
       </div>
 
-      <div data-test-hook="autocomplete">
+        <div data-test-hook="autocomplete">
         <label for="choices-autocomplete">Autocomplete example</label>
         <select
           class="form-control"
@@ -966,7 +968,7 @@ <h2>Select one inputs</h2>
         </script>
       </div>
 
-
+      </div>
     </div>
   </body>
 </html>

src/scripts/choices.ts

@@ -1,4 +1,3 @@
-/* eslint-disable @typescript-eslint/no-explicit-any */
 import { activateChoices, addChoice, removeChoice, filterChoices } from './actions/choices';
 import { addGroup } from './actions/groups';
 import { addItem, highlightItem, removeItem } from './actions/items';
@@ -28,7 +27,7 @@ import Store from './store/store';
 import { coerceBool, mapInputToChoice } from './lib/choice-input';
 import { ChoiceFull } from './interfaces/choice-full';
 import { GroupFull } from './interfaces/group-full';
-import { EventType, KeyCodeMap, PassedElementType, PassedElementTypes } from './interfaces';
+import { EventChoiceValueType, EventType, KeyCodeMap, PassedElementType, PassedElementTypes } from './interfaces';
 import { EventChoice } from './interfaces/event-choice';
 import { NoticeType, NoticeTypes, Templates } from './interfaces/templates';
 import { isHtmlInputElement, isHtmlSelectElement } from './lib/html-guard-statements';
@@ -539,13 +538,10 @@ class Choices {
     return this;
   }
 
-  getValue(valueOnly = false): string[] | EventChoice[] | EventChoice | string {
-    const values = this._store.items.reduce<any[]>((selectedItems, item) => {
-      const itemValue = valueOnly ? item.value : this._getChoiceForOutput(item);
-      selectedItems.push(itemValue);
-
-      return selectedItems;
-    }, []);
+  getValue<B extends boolean = false>(valueOnly?: B): EventChoiceValueType<B> | EventChoiceValueType<B>[] {
+    const values = this._store.items.map((item) => {
+      return (valueOnly ? item.value : this._getChoiceForOutput(item)) as EventChoiceValueType<B>;
+    });
 
     return this._isSelectOneElement || this.config.singleModeForMultiSelect ? values[0] : values;
   }
@@ -788,16 +784,21 @@ class Choices {
 
       this.clearStore(false);
 
-      choicesFromOptions.forEach((groupOrChoice) => {
-        if ('choices' in groupOrChoice) {
-          return;
-        }
-        const choice = groupOrChoice;
+      const updateChoice = (choice: ChoiceFull): void => {
         if (deselectAll) {
           this._store.dispatch(removeItem(choice));
         } else if (existingItems[choice.value]) {
           choice.selected = true;
         }
+      };
+
+      choicesFromOptions.forEach((groupOrChoice) => {
+        if ('choices' in groupOrChoice) {
+          groupOrChoice.choices.forEach(updateChoice);
+
+          return;
+        }
+        updateChoice(groupOrChoice);
       });
 
       /* @todo only generate add events for the added options instead of all
@@ -984,7 +985,7 @@ class Choices {
       if (!this._hasNonChoicePlaceholder && !isSearching && this._isSelectOneElement) {
         // If we have a placeholder choice along with groups
         renderChoices(
-          activeChoices.filter((choice) => choice.placeholder && !choice.groupId),
+          activeChoices.filter((choice) => choice.placeholder && !choice.group),
           false,
           undefined,
         );
@@ -995,6 +996,13 @@ class Choices {
         if (config.shouldSort) {
           activeGroups.sort(config.sorter);
         }
+        // render Choices without group first, regardless of sort, otherwise they won't be distinguishable
+        // from the last group
+        renderChoices(
+          activeChoices.filter((choice) => !choice.placeholder && !choice.group),
+          false,
+          undefined,
+        );
 
         activeGroups.forEach((group) => {
           const groupChoices = renderableChoices(group.choices);
@@ -1160,13 +1168,8 @@ class Choices {
     }
   }
 
-  _getChoiceForOutput(choice?: ChoiceFull, keyCode?: number): EventChoice | undefined {
-    if (!choice) {
-      return undefined;
-    }
-
-    const group = choice.groupId ? this._store.getGroupById(choice.groupId) : null;
-
+  // eslint-disable-next-line class-methods-use-this
+  _getChoiceForOutput(choice: ChoiceFull, keyCode?: number): EventChoice {
     return {
       id: choice.id,
       highlighted: choice.highlighted,
@@ -1178,7 +1181,7 @@ class Choices {
       label: choice.label,
       placeholder: choice.placeholder,
       value: choice.value,
-      groupValue: group && group.label ? group.label : undefined,
+      groupValue: choice.group ? choice.group.label : undefined,
       element: choice.element,
       keyCode,
     };
@@ -2131,7 +2134,7 @@ class Choices {
     group.id = this._lastAddedGroupId;
 
     group.choices.forEach((item: ChoiceFull) => {
-      item.groupId = group.id;
+      item.group = group;
       if (group.disabled) {
         item.disabled = true;
       }

src/scripts/components/wrapped-select.ts

@@ -77,7 +77,7 @@ export default class WrappedSelect extends WrappedElement<HTMLSelectElement> {
 
     return {
       id: 0,
-      groupId: 0,
+      group: null,
       score: 0,
       rank: 0,
       value: option.value,

src/scripts/interfaces/choice-full.ts

@@ -1,7 +1,7 @@
-/* eslint-disable @typescript-eslint/no-explicit-any */
 import { StringUntrusted } from './string-untrusted';
-
-export type CustomProperties = Record<string, any> | string;
+import { Types } from './types';
+// eslint-disable-next-line import/no-cycle
+import { GroupFull } from './group-full';
 
 /*
   A disabled choice appears in the choice dropdown but cannot be selected
@@ -16,11 +16,11 @@ export interface ChoiceFull {
   choiceEl?: HTMLElement;
   labelClass?: Array<string>;
   labelDescription?: string;
-  customProperties?: CustomProperties;
+  customProperties?: Types.CustomProperties;
   disabled: boolean;
   active: boolean;
   elementId?: string;
-  groupId: number;
+  group: GroupFull | null;
   label: StringUntrusted | string;
   placeholder: boolean;
   selected: boolean;

src/scripts/interfaces/event-choice.ts

@@ -1,5 +1,7 @@
 import { InputChoice } from './input-choice';
 
+export type EventChoiceValueType<B extends boolean> = B extends true ? string : EventChoice;
+
 export interface EventChoice extends InputChoice {
   element?: HTMLOptionElement | HTMLOptGroupElement;
   groupValue?: string;

src/scripts/interfaces/group-full.ts

@@ -1,5 +1,4 @@
-/* eslint-disable @typescript-eslint/no-explicit-any */
-
+// eslint-disable-next-line import/no-cycle
 import { ChoiceFull } from './choice-full';
 
 export interface GroupFull {

src/scripts/interfaces/input-choice.ts

@@ -1,17 +1,17 @@
-/* eslint-disable @typescript-eslint/no-explicit-any */
-
 import { StringUntrusted } from './string-untrusted';
+import { Types } from './types';
 
 export interface InputChoice {
   id?: number;
   highlighted?: boolean;
   labelClass?: string | Array<string>;
   labelDescription?: string;
-  customProperties?: Record<string, any> | string;
+  customProperties?: Types.CustomProperties;
   disabled?: boolean;
   active?: boolean;
   label: StringUntrusted | string;
   placeholder?: boolean;
   selected?: boolean;
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
   value: any;
 }

src/scripts/interfaces/input-group.ts

@@ -1,5 +1,3 @@
-/* eslint-disable @typescript-eslint/no-explicit-any */
-
 import { InputChoice } from './input-choice';
 import { StringUntrusted } from './string-untrusted';
 

src/scripts/interfaces/types.ts

@@ -16,4 +16,6 @@ export namespace Types {
     label?: StringUntrusted | string;
   }
   export type ValueOf<T extends object> = T[keyof T];
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  export type CustomProperties = Record<string, any> | string;
 }

src/scripts/lib/choice-input.ts

@@ -64,7 +64,7 @@ export const mapInputToChoice = <T extends string | InputChoice | InputGroup>(
 
   const result: ChoiceFull = {
     id: 0, // actual ID will be assigned during _addChoice
-    groupId: 0, // actual ID will be assigned during _addGroup but before _addChoice
+    group: null, // actual group will be assigned during _addGroup but before _addChoice
     score: 0, // used in search
     rank: 0, // used in search, stable sort order
     value: choice.value,

src/scripts/lib/utils.ts

@@ -1,5 +1,3 @@
-/* eslint-disable @typescript-eslint/no-explicit-any */
-
 import { EventTypes } from '../interfaces/event-type';
 import { StringUntrusted } from '../interfaces/string-untrusted';
 import { StringPreEscaped } from '../interfaces/string-pre-escaped';
@@ -179,6 +177,7 @@ export const cloneObject = <T>(obj: T): T => (obj !== undefined ? JSON.parse(JSO
 /**
  * Returns an array of keys present on the first but missing on the second object
  */
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
 export const diff = (a: Record<string, any>, b: Record<string, any>): string[] => {
   const aKeys = Object.keys(a).sort();
   const bKeys = Object.keys(b).sort();

src/scripts/reducers/choices.ts

@@ -22,6 +22,9 @@ export default function choices(s: StateType, action: ActionTypes, context?: Opt
     case ActionType.REMOVE_CHOICE: {
       action.choice.choiceEl = undefined;
 
+      if (action.choice.group) {
+        action.choice.group.choices = action.choice.group.choices.filter((obj) => obj.id !== action.choice.id);
+      }
       state = state.filter((obj) => obj.id !== action.choice.id);
       break;
     }

src/scripts/reducers/items.ts

@@ -54,8 +54,8 @@ export default function items(s: StateType, action: ActionTypes, context?: Optio
     }
 
     case ActionType.REMOVE_CHOICE: {
-      state = state.filter((item) => item.id !== action.choice.id);
       removeItem(action.choice);
+      state = state.filter((item) => item.id !== action.choice.id);
       break;
     }
 

src/scripts/templates.ts

@@ -268,7 +268,6 @@ const templates: TemplatesInterface = {
       label = escapeForTemplate(allowHTML, label);
       label += ` (${groupName})`;
       label = { trusted: label };
-      div.dataset.groupId = `${choice.groupId}`;
     }
 
     let describedBy: HTMLElement = div;
@@ -300,14 +299,17 @@ const templates: TemplatesInterface = {
       addClassesToElement(div, placeholder);
     }
 
-    div.setAttribute('role', choice.groupId ? 'treeitem' : 'option');
+    div.setAttribute('role', choice.group ? 'treeitem' : 'option');
 
     div.dataset.choice = '';
     div.dataset.id = choice.id as unknown as string;
     div.dataset.value = rawValue;
     if (selectText) {
       div.dataset.selectText = selectText;
     }
+    if (choice.group) {
+      div.dataset.groupId = `${choice.group.id}`;
+    }
 
     assignCustomProperties(div, choice, false);
 

test-e2e/tests/select-multiple.spec.ts

@@ -683,9 +683,10 @@ describe(`Choices - select multiple`, () => {
           await suite.startWithClick();
 
           await expect(suite.dropdown.locator('.choices__group[data-group]')).toHaveCount(1);
-          expect(
-            await suite.selectableChoices.filter({ hasNot: page.locator('[data-group-id]') }).count(),
-          ).toBeGreaterThan(0);
+          expect(await suite.dropdown.locator('.choices__item--choice[data-group-id]').count()).toEqual(1);
+          expect(await suite.dropdown.locator('.choices__item--choice:not([data-group-id])').count()).toBeGreaterThan(
+            1,
+          );
         });
       });
     });
@@ -960,7 +961,8 @@ describe(`Choices - select multiple`, () => {
         await suite.group.locator('.destroy').click({ force: true });
         await suite.advanceClock();
 
-        await expect(suite.group.locator('select > option')).toHaveCount(3);
+        await expect(suite.group.locator('select > optgroup > option')).toHaveCount(1);
+        await expect(suite.group.locator('select > option')).toHaveCount(2);
 
         await suite.group.locator('.init').click({ force: true });
         await suite.advanceClock();

test-e2e/tests/select-one.spec.ts

@@ -528,9 +528,10 @@ describe(`Choices - select one`, () => {
           await suite.startWithClick();
 
           await expect(suite.dropdown.locator('.choices__group[data-group]')).toHaveCount(1);
-          expect(
-            await suite.selectableChoices.filter({ hasNot: page.locator('[data-group-id]') }).count(),
-          ).toBeGreaterThan(0);
+          expect(await suite.dropdown.locator('.choices__item--choice[data-group-id]').count()).toEqual(1);
+          expect(await suite.dropdown.locator('.choices__item--choice:not([data-group-id])').count()).toBeGreaterThan(
+            1,
+          );
         });
       });
     });
@@ -834,7 +835,8 @@ describe(`Choices - select one`, () => {
         await suite.group.locator('.destroy').click({ force: true });
         await suite.advanceClock();
 
-        await expect(suite.group.locator('select > option')).toHaveCount(3);
+        await expect(suite.group.locator('select > optgroup > option')).toHaveCount(1);
+        await expect(suite.group.locator('select > option')).toHaveCount(2);
 
         await suite.group.locator('.init').click({ force: true });
         await suite.advanceClock();