From 92ff5f6da26b7b21649776e5a025992d5cf98fa4 Mon Sep 17 00:00:00 2001 From: Huw Wilkins Date: Tue, 20 Aug 2024 15:04:49 +1000 Subject: [PATCH] refactor: replace internals of MultiSelect with ContextualMenu so it uses a portal --- .../ContextualMenu/ContextualMenu.test.tsx | 6 + .../ContextualMenu/ContextualMenu.tsx | 168 ++++++++++-------- src/components/MultiSelect/MultiSelect.scss | 20 +-- .../MultiSelect/MultiSelect.test.tsx | 8 + src/components/MultiSelect/MultiSelect.tsx | 105 ++++++----- 5 files changed, 171 insertions(+), 136 deletions(-) diff --git a/src/components/ContextualMenu/ContextualMenu.test.tsx b/src/components/ContextualMenu/ContextualMenu.test.tsx index 97235ae4e..1b26c3922 100644 --- a/src/components/ContextualMenu/ContextualMenu.test.tsx +++ b/src/components/ContextualMenu/ContextualMenu.test.tsx @@ -105,6 +105,12 @@ describe("ContextualMenu ", () => { ).not.toBeInTheDocument(); }); + it("can have a custom toggle button", () => { + const label = "Custom toggle"; + render({label}} />); + expect(screen.getByRole("button", { name: label })).toBeInTheDocument(); + }); + it("can not have a toggle button", () => { render(); expect( diff --git a/src/components/ContextualMenu/ContextualMenu.tsx b/src/components/ContextualMenu/ContextualMenu.tsx index d805634f3..fbb656db3 100644 --- a/src/components/ContextualMenu/ContextualMenu.tsx +++ b/src/components/ContextualMenu/ContextualMenu.tsx @@ -1,6 +1,6 @@ import classNames from "classnames"; import React, { useCallback, useEffect, useId, useRef, useState } from "react"; -import type { HTMLProps } from "react"; +import type { HTMLProps, ReactNode } from "react"; import usePortal from "react-useportal"; import { useListener, usePrevious } from "hooks"; import Button from "../Button"; @@ -8,17 +8,18 @@ import type { ButtonProps } from "../Button"; import ContextualMenuDropdown from "./ContextualMenuDropdown"; import type { ContextualMenuDropdownProps } from "./ContextualMenuDropdown"; import type { MenuLink, Position } from "./ContextualMenuDropdown"; -import { ClassName, PropsWithSpread, SubComponentProps } from "types"; +import { + ClassName, + ExclusiveProps, + PropsWithSpread, + SubComponentProps, +} from "types"; export enum Label { Toggle = "Toggle menu", } -/** - * The props for the ContextualMenu component. - * @template L - The type of the link props. - */ -export type Props = PropsWithSpread< +export type BaseProps = PropsWithSpread< { /** * Whether the menu should adjust its horizontal position to fit on the screen. @@ -52,10 +53,6 @@ export type Props = PropsWithSpread< * Additional props to pass to the dropdown. */ dropdownProps?: SubComponentProps; - /** - * Whether the toggle should display a chevron icon. - */ - hasToggleIcon?: boolean; /** * A list of links to display in the menu (if the children prop is not supplied.) */ @@ -76,30 +73,6 @@ export type Props = PropsWithSpread< * Whether the dropdown should scroll if it is too long to fit on the screen. */ scrollOverflow?: boolean; - /** - * The appearance of the toggle button. - */ - toggleAppearance?: ButtonProps["appearance"] | null; - /** - * A class to apply to the toggle button. - */ - toggleClassName?: string | null; - /** - * Whether the toggle button should be disabled. - */ - toggleDisabled?: boolean; - /** - * The toggle button's label. - */ - toggleLabel?: React.ReactNode | null; - /** - * Whether the toggle lable or icon should appear first. - */ - toggleLabelFirst?: boolean; - /** - * Additional props to pass to the toggle button. - */ - toggleProps?: SubComponentProps; /** * Whether the menu should be visible. */ @@ -108,6 +81,45 @@ export type Props = PropsWithSpread< HTMLProps >; +/** + * The props for the ContextualMenu component. + * @template L - The type of the link props. + */ +export type Props = BaseProps & + ExclusiveProps< + { + /** + * Whether the toggle should display a chevron icon. + */ + hasToggleIcon?: boolean; + /** + * The appearance of the toggle button. + */ + toggleAppearance?: ButtonProps["appearance"] | null; + /** + * A class to apply to the toggle button. + */ + toggleClassName?: string | null; + /** + * Whether the toggle button should be disabled. + */ + toggleDisabled?: boolean; + /** + * The toggle button's label. + */ + toggleLabel?: React.ReactNode | null; + /** + * Whether the toggle lable or icon should appear first. + */ + toggleLabelFirst?: boolean; + /** + * Additional props to pass to the toggle button. + */ + toggleProps?: SubComponentProps; + }, + { toggle: ReactNode } + >; + /** * Get the node to use for positioning the menu. * @param wrapper - The component's wrapping element. @@ -159,6 +171,7 @@ const ContextualMenu = ({ position = "right", positionNode, scrollOverflow, + toggle, toggleAppearance, toggleClassName, toggleDisabled, @@ -172,7 +185,6 @@ const ContextualMenu = ({ const wrapper = useRef(null); const [positionCoords, setPositionCoords] = useState(); const [adjustedPosition, setAdjustedPosition] = useState(position); - const hasToggle = hasToggleIcon || toggleLabel; useEffect(() => { setAdjustedPosition(position); @@ -193,14 +205,15 @@ const ContextualMenu = ({ isOpen: visible, onOpen: () => { // Call the toggle callback, if supplied. - onToggleMenu && onToggleMenu(true); + onToggleMenu?.(true); // When the menu opens then update the coordinates of the parent. updatePositionCoords(); }, onClose: () => { // Call the toggle callback, if supplied. - onToggleMenu && onToggleMenu(false); + onToggleMenu?.(false); }, + programmaticallyOpen: true, }); const previousVisible = usePrevious(visible); @@ -272,49 +285,54 @@ const ContextualMenu = ({ useListener(window, onResize, "resize", true, isOpen); useListener(window, onScroll, "scroll", false, isOpen, true); + let toggleNode: ReactNode = null; + if (toggle) { + toggleNode = toggle; + } else if (hasToggleIcon || toggleLabel) { + toggleNode = ( + + ); + } + return ( - {hasToggle ? ( - - ) : null} + {toggleNode} {isOpen && ( diff --git a/src/components/MultiSelect/MultiSelect.scss b/src/components/MultiSelect/MultiSelect.scss index a7551aec8..1cbcdb988 100644 --- a/src/components/MultiSelect/MultiSelect.scss +++ b/src/components/MultiSelect/MultiSelect.scss @@ -10,7 +10,9 @@ $dropdown-max-height: 20rem; // XXX: This is a workaround for https://github.com/canonical/vanilla-framework/issues/5030 @include vf-b-forms; + margin-bottom: $input-margin-bottom; position: relative; + width: 100%; } .multi-select .p-form-validation__message { @@ -26,6 +28,7 @@ $dropdown-max-height: 20rem; .multi-select__input { cursor: pointer; + margin-bottom: 0; position: relative; &.items-selected { @@ -40,19 +43,6 @@ $dropdown-max-height: 20rem; } } -.multi-select__dropdown { - background-color: $colors--theme--background-default; - box-shadow: $box-shadow; - color: $colors--theme--text-default; - left: 0; - max-height: $dropdown-max-height; - overflow: auto; - padding-top: $spv--small; - position: absolute; - right: 0; - top: calc(100% - #{$input-margin-bottom}); -} - .multi-select__dropdown--side-by-side { display: flex; flex-wrap: wrap; @@ -132,6 +122,10 @@ $dropdown-max-height: 20rem; position: relative; z-index: 0; + .multi-select & { + margin-bottom: 0; + } + &::after { content: ""; margin-left: $sph--large; diff --git a/src/components/MultiSelect/MultiSelect.test.tsx b/src/components/MultiSelect/MultiSelect.test.tsx index 47090abd8..b1038af14 100644 --- a/src/components/MultiSelect/MultiSelect.test.tsx +++ b/src/components/MultiSelect/MultiSelect.test.tsx @@ -145,6 +145,14 @@ it("closes the dropdown when clicking outside of the dropdown", async () => { expect(screen.queryByRole("listbox")).not.toBeInTheDocument(); }); +it("closes the dropdown when clicking on the button", async () => { + render(); + await userEvent.click(screen.getByRole("combobox")); + expect(screen.getByRole("listbox")).toBeInTheDocument(); + await userEvent.click(screen.getByRole("combobox")); + expect(screen.queryByRole("listbox")).not.toBeInTheDocument(); +}); + it("updates text in the input field if something is selected", async () => { render( diff --git a/src/components/MultiSelect/MultiSelect.tsx b/src/components/MultiSelect/MultiSelect.tsx index fcced7ad0..73840f9b8 100644 --- a/src/components/MultiSelect/MultiSelect.tsx +++ b/src/components/MultiSelect/MultiSelect.tsx @@ -1,14 +1,8 @@ import type { ReactNode } from "react"; -import React, { useEffect, useId, useMemo, useState } from "react"; +import React, { useEffect, useId, useMemo, useRef, useState } from "react"; import "./MultiSelect.scss"; -import { - Button, - CheckboxInput, - SearchBox, - useClickOutside, - useOnEscapePressed, -} from "../../index"; +import { Button, CheckboxInput, ContextualMenu, SearchBox } from "../../index"; import { FadeInDown } from "./FadeInDown"; @@ -56,7 +50,6 @@ type MultiSelectDropdownProps = { footer?: ReactNode; groupFn?: GroupFn; sortFn?: SortFn; - shouldPinSelectedItems?: boolean; } & React.HTMLAttributes; const sortAlphabetically = (a: MultiSelectItem, b: MultiSelectItem) => { @@ -199,23 +192,10 @@ export const MultiSelect: React.FC = ({ showDropdownFooter = true, variant = "search", }: MultiSelectProps) => { - const wrapperRef = useClickOutside(() => { - setIsDropdownOpen(false); - setFilter(""); - }); - useOnEscapePressed(() => { - setIsDropdownOpen(false); - setFilter(""); - }); + const buttonRef = useRef(); const [isDropdownOpen, setIsDropdownOpen] = useState(false); const [filter, setFilter] = useState(""); - useEffect(() => { - if (!isDropdownOpen) { - setFilter(""); - } - }, [isDropdownOpen]); - const [internalSelectedItems, setInternalSelectedItems] = useState< MultiSelectItem[] >([]); @@ -274,9 +254,22 @@ export const MultiSelect: React.FC = ({ ); } return ( -
-
- {variant === "search" ? ( + { + if (!isOpen) { + setFilter(""); + } + // Handle syncing the state when toggling the menu from within the + // contextual menu component e.g. when clicking outside. + if (isOpen !== isDropdownOpen) { + setIsDropdownOpen(isOpen); + } + }} + position="left" + constrainPanelWidth + toggle={ + variant === "search" ? ( = ({ aria-expanded={isDropdownOpen} className="multi-select__select-button" onClick={() => { - setIsDropdownOpen((isOpen) => !isOpen); + setIsDropdownOpen(!isDropdownOpen); }} + onMouseDown={(event) => { + // If the dropdown is open when this button is clicked the + // click-outside event will fire which will close the dropdown, but + // then the button click event will fire which will immediately + // reopen the dropdown. + // To prevent this we can stop the propagation to the click event + // while `isDropdownOpen` is still set to `true` (by the time we + // get to the `onClick` event `isDropdownOpen` will already be `false`, + // hence having to do this on mouse down). + if (isDropdownOpen) { + event.stopPropagation(); + } + }} + ref={buttonRef} > {listSelected && selectedItems.length > 0 @@ -316,26 +323,28 @@ export const MultiSelect: React.FC = ({ : placeholder ?? "Select items"} - )} - 0 - ? items.filter((item) => - item.label.toLowerCase().includes(filter.toLowerCase()) - ) - : items - } - selectedItems={selectedItems} - disabledItems={disabledItems} - header={dropdownHeader} - updateItems={updateItems} - onSelectItem={onSelectItem} - onDeselectItem={onDeselectItem} - footer={footer} - /> -
-
+ ) + } + visible={isDropdownOpen} + > + 0 + ? items.filter((item) => + item.label.toLowerCase().includes(filter.toLowerCase()) + ) + : items + } + selectedItems={selectedItems} + disabledItems={disabledItems} + header={dropdownHeader} + updateItems={updateItems} + onSelectItem={onSelectItem} + onDeselectItem={onDeselectItem} + footer={footer} + /> + ); };