Merge pull request #383 from eccenca/fix/multiSelectInDevelop-CMEM-7479

Fix MultiSelect component to not set newlySelected incorrectly (CMEM-7479)

Author: haschek

CHANGELOG.md

@@ -33,6 +33,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p
 -   new icons:
     -   `state-confirmed-all`
     -   `state-declined-all`
+-   `<MultiSuggestField />`
+    -   `MultiSuggestFieldSelectionProps` provides `newlyRemoved` for callbacks set when removing a selected item
 
 ### Fixed
 
@@ -57,6 +59,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p
     -   use the latest provided `onChange` function
 -   `<TextField />`, `<TextArea />`
     -   fix emoji false-positives in invisible character detection
+-   `<MultiSuggestField />`
+    -   `onSelection` now sets `newlySelected` only for add actions and no longer sets it to the last element
+    -   border of the BlueprintJS `Tag` elements were fixed
 -   Focus outlines
     -   we use again bold focus outlines for input elements
     -   they are also used for clickable elements that are focused via keyboard (tab navigation)

src/components/MultiSelect/MultiSelect.tsx

@@ -17,11 +17,12 @@ import {
     IconButton,
     MenuItem,
     OverflowText,
-    Spinner
+    Spinner,
 } from "./../../index";
 
 export interface MultiSuggestFieldSelectionProps<T> {
     newlySelected?: T;
+    newlyRemoved?: T;
     selectedItems: T[];
     createdItems: Partial<T>[];
 }
@@ -178,6 +179,8 @@ export function MultiSuggestField<T>({
     intent,
     ...otherMultiSelectProps
 }: MultiSuggestFieldProps<T>) {
+    type SelectionChange = { type: "selected"; item: T } | { type: "removed"; item: T } | { type: "none" };
+
     // Options created by a user
     const createdItems = useRef<T[]>([]);
     // Options passed ouside (f.e. from the backend)
@@ -199,6 +202,7 @@ export function MultiSuggestField<T>({
         query?: string;
         timeoutId?: number;
     }>({});
+    const selectionChange = useRef<SelectionChange>({ type: "none" });
 
     /** Update external items when they change
      *  e.g for auto-complete when query change
@@ -209,11 +213,21 @@ export function MultiSuggestField<T>({
     }, [items.map((item) => itemId(item)).join("|")]);
 
     React.useEffect(() => {
-        onSelection?.({
-            newlySelected: selectedItems.slice(-1)[0],
+        const selectionParams: MultiSuggestFieldSelectionProps<T> = {
             createdItems: createdItems.current,
             selectedItems,
-        });
+        };
+
+        if (selectionChange.current.type === "selected") {
+            selectionParams.newlySelected = selectionChange.current.item;
+        }
+
+        if (selectionChange.current.type === "removed") {
+            selectionParams.newlyRemoved = selectionChange.current.item;
+        }
+
+        onSelection?.(selectionParams);
+        selectionChange.current = { type: "none" };
     }, [
         onSelection,
         selectedItems.map((item) => itemId(item)).join("|"),
@@ -228,6 +242,7 @@ export function MultiSuggestField<T>({
             return;
         }
 
+        selectionChange.current = { type: "none" };
         setSelectedItems(externalSelectedItems);
     }, [externalSelectedItems?.map((item) => itemId(item)).join("|")]);
 
@@ -268,13 +283,18 @@ export function MultiSuggestField<T>({
      * @param matcher
      */
     const removeItemSelection = (matcher: string) => {
-        const filteredItems = selectedItems.filter((item) => itemId(item) !== matcher);
-        setSelectedItems(filteredItems);
+        setSelectedItems((items) => {
+            const removedItem = items.find((item) => itemId(item) === matcher);
+
+            selectionChange.current = removedItem ? { type: "removed", item: removedItem } : { type: "none" };
+
+            return items.filter((item) => itemId(item) !== matcher);
+        });
     };
 
     const defaultFilterPredicate = (item: T, query: string) => {
-        const searchWords = highlighterUtils.extractSearchWords(query, true)
-        return highlighterUtils.matchesAllWords(itemLabel(item).toLowerCase(), searchWords)
+        const searchWords = highlighterUtils.extractSearchWords(query, true);
+        return highlighterUtils.matchesAllWords(itemLabel(item).toLowerCase(), searchWords);
     };
 
     /**
@@ -286,6 +306,7 @@ export function MultiSuggestField<T>({
         if (itemHasBeenSelectedAlready(itemId(item))) {
             removeItemSelection(itemId(item));
         } else {
+            selectionChange.current = { type: "selected", item };
             setSelectedItems((items) => [...items, item]);
         }
 
@@ -365,6 +386,7 @@ export function MultiSuggestField<T>({
     const handleClear = () => {
         requestState.current.query = "";
 
+        selectionChange.current = { type: "none" };
         setSelectedItems([]);
         setFilteredItems([...externalItems, ...createdItems.current]);
     };
@@ -375,7 +397,13 @@ export function MultiSuggestField<T>({
      * @param index
      */
     const removeTagFromSelectionViaIndex = (_label: React.ReactNode, index: number) => {
-        setSelectedItems([...selectedItems.slice(0, index), ...selectedItems.slice(index + 1)]);
+        setSelectedItems((items) => {
+            const removedItem = items[index];
+
+            selectionChange.current = removedItem ? { type: "removed", item: removedItem } : { type: "none" };
+
+            return [...items.slice(0, index), ...items.slice(index + 1)];
+        });
     };
 
     /**

src/components/MultiSuggestField/MultiSuggestField.stories.tsx

@@ -5,6 +5,8 @@ import { Meta, StoryFn } from "@storybook/react";
 import { fn } from "@storybook/test";
 
 import { helpersArgTypes } from "../../../.storybook/helpers";
+import { Notification } from "../Notification/Notification";
+import Spacing from "../Separation/Spacing";
 
 import { MultiSuggestField, MultiSuggestFieldSelectionProps, SimpleDialog } from "./../../../index";
 
@@ -61,7 +63,7 @@ Default.args = {
 
 /**
  * Display always the dropdown after the element was clicked on.
- * Do not wait until the query input was startet.
+ * Do not wait until the query input was started.
  */
 export const dropdownOnFocus = Template.bind({});
 dropdownOnFocus.args = {
@@ -259,3 +261,40 @@ CustomSearch.args = {
         return item.testId.toLowerCase().includes(query) || item.testLabel.toLowerCase().includes(query);
     },
 };
+
+const SelectionNotificationComponent = (): React.JSX.Element => {
+    const [notification, setNotification] = useState<string | null>(null);
+
+    const availableItems = useMemo<string[]>(() => ["existing item"], []);
+
+    const identity = useCallback((item: string): string => item, []);
+
+    const handleOnSelect = useCallback((params: MultiSuggestFieldSelectionProps<string>) => {
+        if (params.newlySelected) {
+            setNotification(`Element added: ${params.newlySelected}`);
+        } else if (params.newlyRemoved) {
+            setNotification(`Element removed: ${params.newlyRemoved}`);
+        }
+    }, []);
+
+    return (
+        <OverlaysProvider>
+            {notification && <Notification intent={"info"}>{notification}</Notification>}
+            <Spacing size={"medium"} />
+            <MultiSuggestField<string>
+                items={availableItems}
+                prePopulateWithItems={true}
+                onSelection={handleOnSelect}
+                itemId={identity}
+                itemLabel={identity}
+                createNewItemFromQuery={identity}
+            />
+        </OverlaysProvider>
+    );
+};
+
+/**
+ * Demonstrates the `newlySelected` and `newlyRemoved` properties of the `onSelection` callback.
+ * A notification appears when an element is added or removed from the selection.
+ */
+export const selectionNotification = SelectionNotificationComponent.bind({});

src/components/MultiSuggestField/tests/MultiSuggestField.test.tsx

@@ -261,6 +261,86 @@ describe("MultiSuggestField", () => {
             });
         });
 
+        it("should set newlySelected only when an item is added", async () => {
+            const onSelection = jest.fn();
+            const initiallySelected = predefinedNotControlledValues.args.selectedItems;
+
+            const { container } = render(
+                <MultiSuggestField
+                    {...dropdownOnFocus.args}
+                    items={items}
+                    selectedItems={initiallySelected}
+                    onSelection={onSelection}
+                />
+            );
+
+            await waitFor(() => {
+                expect(onSelection).toHaveBeenCalledWith({
+                    createdItems: [],
+                    selectedItems: initiallySelected,
+                });
+            });
+
+            onSelection.mockClear();
+
+            const [inputContainer] = container.getElementsByClassName("eccgui-multiselect");
+            const [input] = inputContainer.getElementsByTagName("input");
+
+            fireEvent.click(input);
+
+            await waitFor(() => {
+                const listbox = screen.getByRole("listbox");
+                const menuItems = listbox.getElementsByClassName("eccgui-menu__item");
+
+                expect(menuItems.length).toBe(dropdownOnFocus.args.items.length);
+
+                fireEvent.click(menuItems[2]);
+            });
+
+            await waitFor(() => {
+                expect(onSelection).toHaveBeenLastCalledWith({
+                    createdItems: [],
+                    newlySelected: items[2],
+                    selectedItems: [...initiallySelected, items[2]],
+                });
+            });
+        });
+
+        it("should set newlyRemoved only when an item is removed", async () => {
+            const onSelection = jest.fn();
+            const initiallySelected = predefinedNotControlledValues.args.selectedItems;
+
+            const { container } = render(
+                <MultiSuggestField
+                    {...predefinedNotControlledValues.args}
+                    selectedItems={initiallySelected}
+                    onSelection={onSelection}
+                />
+            );
+
+            await waitFor(() => {
+                expect(onSelection).toHaveBeenCalledWith({
+                    createdItems: [],
+                    selectedItems: initiallySelected,
+                });
+            });
+
+            onSelection.mockClear();
+
+            const [firstTag] = Array.from(container.querySelectorAll("span[data-tag-index]"));
+            const removeTagButton = firstTag.querySelector("button");
+
+            fireEvent.click(removeTagButton!);
+
+            await waitFor(() => {
+                expect(onSelection).toHaveBeenLastCalledWith({
+                    createdItems: [],
+                    newlyRemoved: initiallySelected[0],
+                    selectedItems: initiallySelected.slice(1),
+                });
+            });
+        });
+
         it("should filter items by custom search function", async () => {
             const { container } = render(<MultiSuggestField {...CustomSearch.args} items={items} />);
 
@@ -340,6 +420,13 @@ describe("MultiSuggestField", () => {
                 expect(getByText(firstSelected)).toBeInTheDocument();
                 expect(getByText(secondSelected)).toBeInTheDocument();
             });
+
+            await waitFor(() => {
+                expect(onSelection).toHaveBeenCalledWith({
+                    createdItems: [],
+                    selectedItems: predefinedNotControlledValues.args.selectedItems,
+                });
+            });
         });
 
         it("should call onSelection function with the selected items", async () => {
@@ -485,7 +572,6 @@ describe("MultiSuggestField", () => {
             await waitFor(() => {
                 const expectedObject = {
                     createdItems: [],
-                    newlySelected: items.at(-1),
                     selectedItems: items,
                 };
                 expect(onSelection).toHaveBeenCalledWith(expectedObject);
@@ -513,7 +599,6 @@ describe("MultiSuggestField", () => {
             await waitFor(() => {
                 const expectedObject = {
                     createdItems: [],
-                    newlySelected: items.at(-1),
                     selectedItems: items,
                 };
                 expect(onSelection).toHaveBeenCalledWith(expectedObject);
@@ -536,7 +621,7 @@ describe("MultiSuggestField", () => {
 
                     const expectedObject = {
                         createdItems: [],
-                        newlySelected: selected.at(-1),
+                        newlyRemoved: items[i],
                         selectedItems: selected,
                     };
 

src/components/Tag/tag.scss

@@ -30,8 +30,6 @@ $tag-round-adjustment: 0 !default;
 @import "~@blueprintjs/core/src/components/tag/tag";
 
 .#{$eccgui}-tag__item {
-    --eccgui-tag-border-width: 1px;
-
     flex-grow: 0;
     flex-shrink: 0;
     min-width: calc(#{$tag-height} - 2px);
@@ -141,6 +139,8 @@ $tag-round-adjustment: 0 !default;
 }
 
 .#{$ns}-tag {
+    --eccgui-tag-border-width: 1px;
+
     border-style: solid;
     border-width: var(--eccgui-tag-border-width);