refactor: replace internals of MultiSelect with ContextualMenu so it …

…uses a portal
canonical · Aug 20, 2024 · 92ff5f6 · 92ff5f6
1 parent b716ae7
commit 92ff5f6
Show file tree

Hide file tree

Showing 5 changed files with 171 additions and 136 deletions.
diff --git 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(<ContextualMenu links={[]} toggle={<button>{label}</button>} />);
+    expect(screen.getByRole("button", { name: label })).toBeInTheDocument();
+  });
+
   it("can not have a toggle button", () => {
     render(<ContextualMenu links={[]} />);
     expect(

diff --git a/src/components/ContextualMenu/ContextualMenu.tsx b/src/components/ContextualMenu/ContextualMenu.tsx
@@ -1,24 +1,25 @@
 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";
 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<L> = PropsWithSpread<
+export type BaseProps<L> = PropsWithSpread<
   {
     /**
      * Whether the menu should adjust its horizontal position to fit on the screen.
@@ -52,10 +53,6 @@ export type Props<L> = PropsWithSpread<
      * Additional props to pass to the dropdown.
      */
     dropdownProps?: SubComponentProps<ContextualMenuDropdownProps>;
-    /**
-     * 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<L> = 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<ButtonProps>;
     /**
      * Whether the menu should be visible.
      */
@@ -108,6 +81,45 @@ export type Props<L> = PropsWithSpread<
   HTMLProps<HTMLSpanElement>
 >;
 
+/**
+ * The props for the ContextualMenu component.
+ * @template L - The type of the link props.
+ */
+export type Props<L> = BaseProps<L> &
+  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<ButtonProps>;
+    },
+    { toggle: ReactNode }
+  >;
+
 /**
  * Get the node to use for positioning the menu.
  * @param wrapper - The component's wrapping element.
@@ -159,6 +171,7 @@ const ContextualMenu = <L,>({
   position = "right",
   positionNode,
   scrollOverflow,
+  toggle,
   toggleAppearance,
   toggleClassName,
   toggleDisabled,
@@ -172,7 +185,6 @@ const ContextualMenu = <L,>({
   const wrapper = useRef<HTMLDivElement | null>(null);
   const [positionCoords, setPositionCoords] = useState<DOMRect>();
   const [adjustedPosition, setAdjustedPosition] = useState(position);
-  const hasToggle = hasToggleIcon || toggleLabel;
 
   useEffect(() => {
     setAdjustedPosition(position);
@@ -193,14 +205,15 @@ const ContextualMenu = <L,>({
     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 = <L,>({
   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 = (
+      <Button
+        appearance={toggleAppearance}
+        aria-controls={id}
+        aria-expanded={isOpen ? "true" : "false"}
+        aria-label={toggleLabel ? null : Label.Toggle}
+        aria-pressed={isOpen ? "true" : "false"}
+        aria-haspopup="true"
+        className={classNames("p-contextual-menu__toggle", toggleClassName)}
+        disabled={toggleDisabled}
+        hasIcon={hasToggleIcon}
+        onClick={(evt: React.MouseEvent) => {
+          if (!isOpen) {
+            openPortal(evt);
+          } else {
+            closePortal(evt);
+          }
+        }}
+        type="button"
+        {...toggleProps}
+      >
+        {toggleLabelFirst ? labelNode : null}
+        {hasToggleIcon ? (
+          <i
+            className={classNames(
+              "p-icon--chevron-down p-contextual-menu__indicator",
+              {
+                "is-light": ["negative", "positive"].includes(toggleAppearance),
+              }
+            )}
+          ></i>
+        ) : null}
+        {toggleLabelFirst ? null : labelNode}
+      </Button>
+    );
+  }
+
   return (
     <span
       className={contextualMenuClassName}
       ref={wrapperRef}
       {...wrapperProps}
     >
-      {hasToggle ? (
-        <Button
-          appearance={toggleAppearance}
-          aria-controls={id}
-          aria-expanded={isOpen ? "true" : "false"}
-          aria-label={toggleLabel ? null : Label.Toggle}
-          aria-pressed={isOpen ? "true" : "false"}
-          aria-haspopup="true"
-          className={classNames("p-contextual-menu__toggle", toggleClassName)}
-          disabled={toggleDisabled}
-          hasIcon={hasToggleIcon}
-          onClick={(evt: React.MouseEvent) => {
-            if (!isOpen) {
-              openPortal(evt);
-            } else {
-              closePortal(evt);
-            }
-          }}
-          type="button"
-          {...toggleProps}
-        >
-          {toggleLabelFirst ? labelNode : null}
-          {hasToggleIcon ? (
-            <i
-              className={classNames(
-                "p-icon--chevron-down p-contextual-menu__indicator",
-                {
-                  "is-light": ["negative", "positive"].includes(
-                    toggleAppearance
-                  ),
-                }
-              )}
-            ></i>
-          ) : null}
-          {toggleLabelFirst ? null : labelNode}
-        </Button>
-      ) : null}
+      {toggleNode}
       {isOpen && (
         <Portal>
           <ContextualMenuDropdown<L>

diff --git 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
@@ -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(<MultiSelect items={items} />);
+  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(
     <MultiSelect items={items} selectedItems={[items[0]]} variant="condensed" />