From 02a6069b8bcd5b5b46b12b476b049f894144115e Mon Sep 17 00:00:00 2001 From: Doug MacKenzie Date: Mon, 4 Dec 2023 14:51:32 +1100 Subject: [PATCH] Release Multi Select (#4367) * Fix accessible name for clear all button * Move stories from staging to components * Add changeset * Export MultiSelect from components/index * Update tests after clear all accessible name change * Uncomment tests for clear all on MultiSelectToggle spec * Update selector for single clear button * Adjust wording to Remove all options from {label} * Remove space from name in stories * Move focus to dialog on open * Disable focus lock by default on Popover * Move focus shift out of Popover and into MultiSelect * Adjust test to confirm dialog has focus on open * Adjust Select specs back to using toBeVisible * Rename subcomponent story titles * Fix Select tests --- .changeset/shiny-bobcats-cry.md | 5 + .../src/MultiSelect/MultiSelect.spec.tsx | 13 +- .../src/MultiSelect/MultiSelect.tsx | 2 + .../MultiSelect.stickersheet.stories.tsx | 2 +- .../MultiSelect/_docs/MultiSelect.stories.tsx | 2 +- .../_docs/Checkbox.stickersheet.stories.tsx | 2 +- .../Checkbox/_docs/Checkbox.stories.tsx | 2 +- ...SelectOptionField.stickersheet.stories.tsx | 2 +- .../_docs/MultiSelectOptionField.stories.tsx | 2 +- ...ultiSelectOptions.stickersheet.stories.tsx | 2 +- .../_docs/MultiSelectOptions.stories.tsx | 2 +- .../MultiSelectToggle.spec.tsx | 65 ++++---- .../MultiSelectToggle/MultiSelectToggle.tsx | 151 +++++++++--------- ...MultiSelectToggle.stickersheet.stories.tsx | 2 +- .../subcomponents/Popover/Popover.module.scss | 7 + .../subcomponents/Popover/Popover.tsx | 3 +- .../_docs/Popover.stickersheet.stories.tsx | 2 +- .../Popover/_docs/Popover.stories.tsx | 2 +- .../src/__future__/Select/Select.spec.tsx | 14 +- packages/components/src/index.ts | 1 + 20 files changed, 152 insertions(+), 131 deletions(-) create mode 100644 .changeset/shiny-bobcats-cry.md diff --git a/.changeset/shiny-bobcats-cry.md b/.changeset/shiny-bobcats-cry.md new file mode 100644 index 00000000000..637fd6f68f6 --- /dev/null +++ b/.changeset/shiny-bobcats-cry.md @@ -0,0 +1,5 @@ +--- +"@kaizen/components": minor +--- + +Release Multi Select component diff --git a/packages/components/src/MultiSelect/MultiSelect.spec.tsx b/packages/components/src/MultiSelect/MultiSelect.spec.tsx index e98a8fac26a..f095bf124fe 100644 --- a/packages/components/src/MultiSelect/MultiSelect.spec.tsx +++ b/packages/components/src/MultiSelect/MultiSelect.spec.tsx @@ -212,7 +212,9 @@ describe("", () => { expect(popover).toBeVisible() }) - await user.click(getByRole("button", { name: "Clear all waffles" })) + await user.click( + getByRole("button", { name: "Remove all options from Jalapeno" }) + ) await waitFor(() => { expect(popover).toBeVisible() @@ -230,7 +232,8 @@ describe("", () => { /> ) const toggleButton = getByRole("button", { name: "Jalapeno" }) - expect(toggleButton).toHaveFocus() + const dialog = getByRole("dialog") + expect(dialog).toHaveFocus() await user.tab() await waitFor(() => { @@ -263,7 +266,7 @@ describe("", () => { await user.tab() await waitFor(() => { expect( - getByRole("button", { name: "Clear all waffles" }) + getByRole("button", { name: "Remove all options from Jalapeno" }) ).toHaveFocus() }) }) @@ -323,7 +326,9 @@ describe("Removing all options", () => { const pancakesOption = getByText("Pancakes") const waffleOption = getByText("Waffle") - const clearAllButton = getByRole("button", { name: "Clear all waffles" }) + const clearAllButton = getByRole("button", { + name: "Remove all options from Jalapeno", + }) await user.click(clearAllButton) await waitFor(() => { expect(pancakesOption).not.toBeInTheDocument() diff --git a/packages/components/src/MultiSelect/MultiSelect.tsx b/packages/components/src/MultiSelect/MultiSelect.tsx index e41bbee77d1..5c28df2426a 100644 --- a/packages/components/src/MultiSelect/MultiSelect.tsx +++ b/packages/components/src/MultiSelect/MultiSelect.tsx @@ -115,10 +115,12 @@ export const MultiSelect = ({ refs={refs} id={`${id}--popover`} focusOnProps={{ + enabled: true, onClickOutside, onEscapeKey: handleClose, shards: [toggleButtonRef], noIsolation: true, + onActivation: (): void => refs.floating?.current?.focus(), }} classNameOverride={styles.popover} > diff --git a/packages/components/src/MultiSelect/_docs/MultiSelect.stickersheet.stories.tsx b/packages/components/src/MultiSelect/_docs/MultiSelect.stickersheet.stories.tsx index f318faa471f..e561b0bce44 100644 --- a/packages/components/src/MultiSelect/_docs/MultiSelect.stickersheet.stories.tsx +++ b/packages/components/src/MultiSelect/_docs/MultiSelect.stickersheet.stories.tsx @@ -11,7 +11,7 @@ import { MultiSelect, MultiSelectProps } from "../index" const IS_CHROMATIC = isChromatic() export default { - title: "Staging/Multi Select", + title: "Components/MultiSelect", parameters: { chromatic: { disable: false }, controls: { disable: true }, diff --git a/packages/components/src/MultiSelect/_docs/MultiSelect.stories.tsx b/packages/components/src/MultiSelect/_docs/MultiSelect.stories.tsx index 945c2c44e97..b6f5939dbdb 100644 --- a/packages/components/src/MultiSelect/_docs/MultiSelect.stories.tsx +++ b/packages/components/src/MultiSelect/_docs/MultiSelect.stories.tsx @@ -3,7 +3,7 @@ import { Meta, StoryObj } from "@storybook/react" import { MultiSelect, MultiSelectProps } from "../index" const meta = { - title: "Staging/Multi Select", + title: "Components/MultiSelect", component: MultiSelect, argTypes: { selectedValues: { diff --git a/packages/components/src/MultiSelect/subcomponents/Checkbox/_docs/Checkbox.stickersheet.stories.tsx b/packages/components/src/MultiSelect/subcomponents/Checkbox/_docs/Checkbox.stickersheet.stories.tsx index 1b517d6e4b4..9f09eb1a6d8 100644 --- a/packages/components/src/MultiSelect/subcomponents/Checkbox/_docs/Checkbox.stickersheet.stories.tsx +++ b/packages/components/src/MultiSelect/subcomponents/Checkbox/_docs/Checkbox.stickersheet.stories.tsx @@ -7,7 +7,7 @@ import { import { Checkbox, CheckedStatus } from "../index" export default { - title: "Staging/Multi Select/Checkbox", + title: "Components/MultiSelect/Checkbox", parameters: { chromatic: { disable: false }, controls: { disable: true }, diff --git a/packages/components/src/MultiSelect/subcomponents/Checkbox/_docs/Checkbox.stories.tsx b/packages/components/src/MultiSelect/subcomponents/Checkbox/_docs/Checkbox.stories.tsx index 0c103697dee..85e27eb5888 100644 --- a/packages/components/src/MultiSelect/subcomponents/Checkbox/_docs/Checkbox.stories.tsx +++ b/packages/components/src/MultiSelect/subcomponents/Checkbox/_docs/Checkbox.stories.tsx @@ -3,7 +3,7 @@ import { Meta, StoryObj } from "@storybook/react" import { Checkbox, CheckedStatus } from "../index" const meta = { - title: "Staging/Multi Select/Checkbox", + title: "Components/MultiSelect/Checkbox", component: Checkbox, args: { checkedStatus: "unchecked", diff --git a/packages/components/src/MultiSelect/subcomponents/MultiSelectOptionField/_docs/MultiSelectOptionField.stickersheet.stories.tsx b/packages/components/src/MultiSelect/subcomponents/MultiSelectOptionField/_docs/MultiSelectOptionField.stickersheet.stories.tsx index aa8bbf0b430..ddc787535f5 100644 --- a/packages/components/src/MultiSelect/subcomponents/MultiSelectOptionField/_docs/MultiSelectOptionField.stickersheet.stories.tsx +++ b/packages/components/src/MultiSelect/subcomponents/MultiSelectOptionField/_docs/MultiSelectOptionField.stickersheet.stories.tsx @@ -5,7 +5,7 @@ import { StickerSheet } from "~storybook/components/StickerSheet" import { MultiSelectOptionField, MultiSelectOptionFieldProps } from "../index" const meta = { - title: "Staging/Multi Select/MultiSelectOptionField", + title: "Components/MultiSelect/MultiSelectOptionField", parameters: { chromatic: { disable: false }, controls: { disable: true }, diff --git a/packages/components/src/MultiSelect/subcomponents/MultiSelectOptionField/_docs/MultiSelectOptionField.stories.tsx b/packages/components/src/MultiSelect/subcomponents/MultiSelectOptionField/_docs/MultiSelectOptionField.stories.tsx index 6af9e11945f..4feef367048 100644 --- a/packages/components/src/MultiSelect/subcomponents/MultiSelectOptionField/_docs/MultiSelectOptionField.stories.tsx +++ b/packages/components/src/MultiSelect/subcomponents/MultiSelectOptionField/_docs/MultiSelectOptionField.stories.tsx @@ -5,7 +5,7 @@ import { CheckboxProps } from "../../Checkbox" import { MultiSelectOptionField } from "../index" const meta = { - title: "Staging/Multi Select/MultiSelectOptionField", + title: "Components/MultiSelect/MultiSelectOptionField", component: MultiSelectOptionField, args: { id: "id--multi-select-option-field", diff --git a/packages/components/src/MultiSelect/subcomponents/MultiSelectOptions/_docs/MultiSelectOptions.stickersheet.stories.tsx b/packages/components/src/MultiSelect/subcomponents/MultiSelectOptions/_docs/MultiSelectOptions.stickersheet.stories.tsx index cfd211e04ee..7e91904e9ed 100644 --- a/packages/components/src/MultiSelect/subcomponents/MultiSelectOptions/_docs/MultiSelectOptions.stickersheet.stories.tsx +++ b/packages/components/src/MultiSelect/subcomponents/MultiSelectOptions/_docs/MultiSelectOptions.stickersheet.stories.tsx @@ -7,7 +7,7 @@ import { import { MultiSelectOptions, MultiSelectOptionsProps } from "../index" export default { - title: "Staging/Multi Select/MultiSelectOptions", + title: "Components/MultiSelect/MultiSelectOptions", parameters: { chromatic: { disable: false }, controls: { disable: true }, diff --git a/packages/components/src/MultiSelect/subcomponents/MultiSelectOptions/_docs/MultiSelectOptions.stories.tsx b/packages/components/src/MultiSelect/subcomponents/MultiSelectOptions/_docs/MultiSelectOptions.stories.tsx index cb18ff7a6f6..13ea2333e64 100644 --- a/packages/components/src/MultiSelect/subcomponents/MultiSelectOptions/_docs/MultiSelectOptions.stories.tsx +++ b/packages/components/src/MultiSelect/subcomponents/MultiSelectOptions/_docs/MultiSelectOptions.stories.tsx @@ -3,7 +3,7 @@ import { Meta, StoryObj } from "@storybook/react" import { MultiSelectOptions, MultiSelectOptionsProps } from "../index" const meta = { - title: "Staging/Multi Select/MultiSelectOptions", + title: "Components/MultiSelect/MultiSelectOptions", component: MultiSelectOptions, args: { id: "id--multi-select-options", diff --git a/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/MultiSelectToggle.spec.tsx b/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/MultiSelectToggle.spec.tsx index ee95215cb16..ba4b29822bd 100644 --- a/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/MultiSelectToggle.spec.tsx +++ b/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/MultiSelectToggle.spec.tsx @@ -70,42 +70,39 @@ describe("", () => { }) describe("Has no selected options", () => { - // @todo: re-enable test when Clear All functionality implemented - // eslint-disable-next-line jest/no-commented-out-tests - // it("does not show the clear all button", () => { - // const { queryByRole } = render() - // expect( - // queryByRole("button", { name: "Clear all waffles" }) - // ).not.toBeInTheDocument() - // }) + it("does not show the clear all button", () => { + const { queryByRole } = render() + expect( + queryByRole("button", { name: "Remove all options from Waffle" }) + ).not.toBeInTheDocument() + }) }) describe("Has selected options", () => { - // @todo: re-enable test when Clear option functionality implemented - // eslint-disable-next-line jest/no-commented-out-tests - // it("does not call onClick when clearing a single selected item", async () => { - // const { getByRole } = render( - // - // ) - // await user.click(getByRole("button", { name: "Clear waffle" })) - // await waitFor(() => { - // expect(onClick).not.toHaveBeenCalled() - // }) - // }) - // @todo: re-enable test when Clear All functionality implemented - // eslint-disable-next-line jest/no-commented-out-tests - // it("does not call onClick when clearing all selected items", async () => { - // const { getByRole } = render( - // - // ) - // await user.click(getByRole("button", { name: "Clear all waffles" })) - // await waitFor(() => { - // expect(onClick).not.toHaveBeenCalled() - // }) - // }) + it("does not call onClick when clearing a single selected item", async () => { + const { getByRole } = render( + + ) + await user.click(getByRole("button", { name: "Remove option: Waffle" })) + await waitFor(() => { + expect(onClick).not.toHaveBeenCalled() + }) + }) + + it("does not call onClick when clearing all selected items", async () => { + const { getByRole } = render( + + ) + await user.click( + getByRole("button", { name: "Remove all options from Waffle" }) + ) + await waitFor(() => { + expect(onClick).not.toHaveBeenCalled() + }) + }) }) }) diff --git a/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/MultiSelectToggle.tsx b/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/MultiSelectToggle.tsx index 6e5d38a473b..e370b5af052 100644 --- a/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/MultiSelectToggle.tsx +++ b/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/MultiSelectToggle.tsx @@ -1,4 +1,4 @@ -import React, { HTMLAttributes, forwardRef } from "react" +import React, { HTMLAttributes, forwardRef, useId } from "react" import classnames from "classnames" import { ClearButton } from "~components/ClearButton" import { FieldMessageProps } from "~components/FieldMessage" @@ -38,84 +38,89 @@ export const MultiSelectToggle = forwardRef< ...restProps }, ref - ): JSX.Element => ( - <> - {/* - * The inner - + ): JSX.Element => { + const clearAllId = useId() + return ( + <> + {/* + * The inner - { - e.stopPropagation() - onRemoveAllOptions() - }} - /> - - )} +
+ {selectedOptions.length > 0 && ( + <> + {/* list-style: none removes role="list" in Safari */} + {/* eslint-disable-next-line jsx-a11y/no-redundant-roles */} +
    + {selectedOptions.map(({ label, value }) => ( + // This stops the underlying toggle collapsing the popover when interactive with Tags + // eslint-disable-next-line jsx-a11y/no-noninteractive-element-interactions, jsx-a11y/click-events-have-key-events +
  • e.stopPropagation()}> + onRemoveOption(value), + }} + > + {label} + +
    • + ))} +
+ + { + e.stopPropagation() + onRemoveAllOptions() + }} + /> + + )} +
- - - ) + + ) + } ) MultiSelectToggle.displayName = "MultiSelectToggle" diff --git a/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/_docs/MultiSelectToggle.stickersheet.stories.tsx b/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/_docs/MultiSelectToggle.stickersheet.stories.tsx index ea9ccaac5fc..80a8a44b5da 100644 --- a/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/_docs/MultiSelectToggle.stickersheet.stories.tsx +++ b/packages/components/src/MultiSelect/subcomponents/MultiSelectToggle/_docs/MultiSelectToggle.stickersheet.stories.tsx @@ -7,7 +7,7 @@ import { import { MultiSelectToggle, MultiSelectToggleProps } from "../index" export default { - title: "Staging/Multi Select/MultiSelectToggle", + title: "Components/MultiSelect/MultiSelectToggle", parameters: { chromatic: { disable: false }, controls: { disable: true }, diff --git a/packages/components/src/MultiSelect/subcomponents/Popover/Popover.module.scss b/packages/components/src/MultiSelect/subcomponents/Popover/Popover.module.scss index cc43abd61cc..adb65017eec 100644 --- a/packages/components/src/MultiSelect/subcomponents/Popover/Popover.module.scss +++ b/packages/components/src/MultiSelect/subcomponents/Popover/Popover.module.scss @@ -5,7 +5,14 @@ .popover { box-sizing: border-box; box-shadow: $shadow-large-box-shadow; + border: $border-focus-ring-border-width $border-focus-ring-border-style + transparent; border-radius: $border-solid-border-radius; background: $color-white; overflow: auto; + + &:focus { + outline: none; + border-color: $color-blue-500; + } } diff --git a/packages/components/src/MultiSelect/subcomponents/Popover/Popover.tsx b/packages/components/src/MultiSelect/subcomponents/Popover/Popover.tsx index 2445f863369..6f3f44b1f5f 100644 --- a/packages/components/src/MultiSelect/subcomponents/Popover/Popover.tsx +++ b/packages/components/src/MultiSelect/subcomponents/Popover/Popover.tsx @@ -60,13 +60,14 @@ export const Popover = ({ }) return createPortal( - +
{children} diff --git a/packages/components/src/MultiSelect/subcomponents/Popover/_docs/Popover.stickersheet.stories.tsx b/packages/components/src/MultiSelect/subcomponents/Popover/_docs/Popover.stickersheet.stories.tsx index c546c53f99d..69e43a82f4b 100644 --- a/packages/components/src/MultiSelect/subcomponents/Popover/_docs/Popover.stickersheet.stories.tsx +++ b/packages/components/src/MultiSelect/subcomponents/Popover/_docs/Popover.stickersheet.stories.tsx @@ -6,7 +6,7 @@ import { StickerSheetStory } from "~storybook/components/StickerSheet" import { Popover, PopoverProps, useFloating } from "../index" export default { - title: "Staging/Multi Select/Popover", + title: "Components/MultiSelect/Popover", parameters: { chromatic: { disable: false }, controls: { disable: true }, diff --git a/packages/components/src/MultiSelect/subcomponents/Popover/_docs/Popover.stories.tsx b/packages/components/src/MultiSelect/subcomponents/Popover/_docs/Popover.stories.tsx index 88e73b86f15..84a5f11a9b5 100644 --- a/packages/components/src/MultiSelect/subcomponents/Popover/_docs/Popover.stories.tsx +++ b/packages/components/src/MultiSelect/subcomponents/Popover/_docs/Popover.stories.tsx @@ -3,7 +3,7 @@ import { Meta, StoryObj } from "@storybook/react" import { Popover, useFloating } from "../index" const meta = { - title: "Staging/Multi Select/Popover", + title: "Components/MultiSelect/Popover", component: Popover, args: { refs: undefined, diff --git a/packages/components/src/__future__/Select/Select.spec.tsx b/packages/components/src/__future__/Select/Select.spec.tsx index 55025ced5f2..1cd2c31be3f 100644 --- a/packages/components/src/__future__/Select/Select.spec.tsx +++ b/packages/components/src/__future__/Select/Select.spec.tsx @@ -315,13 +315,14 @@ describe("", () => { it("fires onSelectionChange when hits enter on a option", async () => { const spy = jest.fn() - const { getByRole } = render( - - ) - const trigger = getByRole("combobox", { name: "Mock Label" }) + const { getByRole } = render() await user.tab() + await user.keyboard("{ArrowDown}") await waitFor(() => { expect( getByRole("option", { name: "Short black", selected: false }) - ).toHaveFocus() + ).toBeVisible() }) await user.keyboard("{Enter}") await waitFor(() => { expect(spy).toHaveBeenCalledTimes(1) - expect(trigger).toHaveAccessibleName("Mock Label") }) }) }) diff --git a/packages/components/src/index.ts b/packages/components/src/index.ts index 530402423e6..7bb6994645a 100644 --- a/packages/components/src/index.ts +++ b/packages/components/src/index.ts @@ -33,6 +33,7 @@ export * from "./LikertScaleLegacy" export * from "./Loading" export * from "./Menu" export * from "./Modal" +export * from "./MultiSelect" export * from "./Notification" export * from "./Pagination" export * from "./Popover"