From 907925c702f35f05acf685c53a304c139b75f523 Mon Sep 17 00:00:00 2001 From: sirineJ <112706079+sirineJ@users.noreply.github.com> Date: Thu, 26 Dec 2024 15:44:26 +0100 Subject: [PATCH] Edit popover component --- .../components/Popover/Popover.module.css | 60 +-- .../components/Popover/Popover.spec.tsx | 142 +----- .../components/Popover/Popover.stories.tsx | 8 +- .../circuit-ui/components/Popover/Popover.tsx | 439 +++++++----------- .../circuit-ui/components/Popover/index.tsx | 2 +- packages/circuit-ui/index.ts | 5 +- 6 files changed, 216 insertions(+), 440 deletions(-) diff --git a/packages/circuit-ui/components/Popover/Popover.module.css b/packages/circuit-ui/components/Popover/Popover.module.css index 44540369e1..455f3e2619 100644 --- a/packages/circuit-ui/components/Popover/Popover.module.css +++ b/packages/circuit-ui/components/Popover/Popover.module.css @@ -17,77 +17,33 @@ display: inline-block; } -.menu { +.content { box-sizing: border-box; max-height: var(--popover-max-height); - padding: var(--cui-spacings-byte) 0; + padding: 0; + margin: 0; overflow-y: auto; + pointer-events: none; visibility: hidden; background-color: var(--cui-bg-elevated); - border: 1px solid var(--cui-border-subtle); + border: none; border-radius: var(--cui-border-radius-byte); box-shadow: 0 3px 8px 0 rgb(0 0 0 / 20%); opacity: 0; } -@media (max-width: 479px) { - .menu { - border-bottom-right-radius: 0; - border-bottom-left-radius: 0; - opacity: 1; - transition: - transform var(--cui-transitions-default), - visibility var(--cui-transitions-default); - transform: translateY(100%); - } -} - -.menu.open { +.content.open { + pointer-events: all; visibility: inherit; opacity: 1; } -@media (max-width: 479px) { - .menu.open { - transform: translateY(0); - } -} - .divider { width: calc(100% - var(--cui-spacings-mega) * 2); margin: var(--cui-spacings-byte) var(--cui-spacings-mega); } -@media (max-width: 479px) { - .overlay { - position: fixed; - top: 0; - right: 0; - bottom: 0; - left: 0; - visibility: hidden; - background-color: var(--cui-bg-overlay); - opacity: 0; - transition: - opacity var(--cui-transitions-default), - visibility var(--cui-transitions-default); - } - - .overlay.open { - visibility: inherit; - opacity: 1; - } -} - -.wrapper { - pointer-events: none; -} - -.wrapper.open { - pointer-events: all; -} - -.wrapper.open::after { +.content.open::after { position: absolute; right: 0; bottom: 0; diff --git a/packages/circuit-ui/components/Popover/Popover.spec.tsx b/packages/circuit-ui/components/Popover/Popover.spec.tsx index 3106d3ecf6..98f1a0088f 100644 --- a/packages/circuit-ui/components/Popover/Popover.spec.tsx +++ b/packages/circuit-ui/components/Popover/Popover.spec.tsx @@ -13,78 +13,12 @@ * limitations under the License. */ -import type { FC } from 'react'; import { afterEach, describe, expect, it, vi } from 'vitest'; -import { Delete, Add, Download, type IconProps } from '@sumup-oss/icons'; - -import { - act, - axe, - render, - userEvent, - screen, - type RenderFn, -} from '../../util/test-utils.js'; -import type { ClickEvent } from '../../types/events.js'; - -import { - PopoverItem, - type PopoverItemProps, - Popover, - type PopoverProps, -} from './Popover.js'; - -describe('PopoverItem', () => { - function renderPopoverItem( - renderFn: RenderFn, - props: PopoverItemProps, - ) { - return renderFn(); - } - - const baseProps = { - children: 'PopoverItem', - icon: Download as FC, - }; - - describe('Styles', () => { - it('should render as Link when an href (and onClick) is passed', () => { - const props = { - ...baseProps, - href: 'https://sumup.com', - onClick: vi.fn(), - }; - const { container } = renderPopoverItem(render, props); - const anchorEl = container.querySelector('a'); - expect(anchorEl).toBeVisible(); - }); +import { createRef } from 'react'; - it('should render as a `button` when an onClick is passed', () => { - const props = { ...baseProps, onClick: vi.fn() }; - const { container } = renderPopoverItem(render, props); - const buttonEl = container.querySelector('button'); - expect(buttonEl).toBeVisible(); - }); - }); +import { act, axe, render, userEvent, screen } from '../../util/test-utils.js'; - describe('Logic', () => { - it('should call onClick when rendered as Link', async () => { - const props = { - ...baseProps, - href: 'https://sumup.com', - onClick: vi.fn((event: ClickEvent) => { - event.preventDefault(); - }), - }; - const { container } = renderPopoverItem(render, props); - const anchorEl = container.querySelector('a'); - if (anchorEl) { - await userEvent.click(anchorEl); - } - expect(props.onClick).toHaveBeenCalledTimes(1); - }); - }); -}); +import { Popover, type PopoverProps } from './Popover.js'; describe('Popover', () => { afterEach(() => { @@ -117,23 +51,18 @@ describe('Popover', () => { const baseProps: PopoverProps = { component: (triggerProps) => , - actions: [ - { - onClick: vi.fn(), - children: 'Add', - icon: Add as FC, - }, - { type: 'divider' }, - { - onClick: vi.fn(), - children: 'Remove', - icon: Delete as FC, - destructive: true, - }, - ], + children:
Popover content
, isOpen: true, onToggle: vi.fn(createStateSetter(true)), }; + + it('should forward a ref', () => { + const ref = createRef(); + renderPopover({ ...baseProps, ref }); + const dialog = screen.getByRole('dialog', { hidden: true }); + expect(ref.current).toBe(dialog); + }); + it('should open the popover when clicking the trigger element', async () => { const isOpen = false; const onToggle = vi.fn(createStateSetter(isOpen)); @@ -211,29 +140,26 @@ describe('Popover', () => { expect(baseProps.onToggle).toHaveBeenCalledTimes(1); }); - it('should close the popover when clicking a popover item', async () => { - renderPopover(baseProps); - - const popoverItems = screen.getAllByRole('menuitem'); - - await userEvent.click(popoverItems[0]); - - expect(baseProps.onToggle).toHaveBeenCalledTimes(1); - }); - it('should move focus to the first popover item after opening', async () => { const isOpen = false; const onToggle = vi.fn(createStateSetter(isOpen)); - const { rerender } = renderPopover({ + const props = { ...baseProps, isOpen, onToggle, - }); + children: ( +
+ + +
+ ), + }; + const { rerender } = renderPopover(props); - rerender(); + rerender(); - const popoverItems = screen.getAllByRole('menuitem'); + const popoverItems = screen.getAllByText('Item'); expect(popoverItems[0]).toHaveFocus(); @@ -261,19 +187,8 @@ describe('Popover', () => { }); }); - it('should render the popover with menu semantics by default ', async () => { - renderPopover(baseProps); - - const menu = screen.getByRole('menu'); - expect(menu).toBeVisible(); - const menuitems = screen.getAllByRole('menuitem'); - expect(menuitems.length).toBe(2); - - await flushMicrotasks(); - }); - it('should render the popover without menu semantics ', async () => { - renderPopover({ ...baseProps, role: null }); + renderPopover({ ...baseProps }); const menu = screen.queryByRole('menu'); expect(menu).toBeNull(); @@ -282,13 +197,4 @@ describe('Popover', () => { await flushMicrotasks(); }); - - it('should hide dividers from the accessibility tree', async () => { - const { baseElement } = renderPopover(baseProps); - - const dividers = baseElement.querySelectorAll('hr[aria-hidden="true"'); - expect(dividers.length).toBe(1); - - await flushMicrotasks(); - }); }); diff --git a/packages/circuit-ui/components/Popover/Popover.stories.tsx b/packages/circuit-ui/components/Popover/Popover.stories.tsx index 453d660858..d03d7f4b9d 100644 --- a/packages/circuit-ui/components/Popover/Popover.stories.tsx +++ b/packages/circuit-ui/components/Popover/Popover.stories.tsx @@ -70,7 +70,9 @@ export const Base = (args: PopoverProps) => { Open popover )} - /> + > +
Hello 👋
+ ); }; @@ -93,7 +95,9 @@ export const Offset = (args: PopoverProps) => { Open popover )} - /> + > +
Hello 👋
+ ); }; diff --git a/packages/circuit-ui/components/Popover/Popover.tsx b/packages/circuit-ui/components/Popover/Popover.tsx index e0ab851875..218f08233a 100644 --- a/packages/circuit-ui/components/Popover/Popover.tsx +++ b/packages/circuit-ui/components/Popover/Popover.tsx @@ -15,16 +15,17 @@ 'use client'; -import type React from 'react'; import { + type ReactNode, Fragment, useCallback, useEffect, useId, useRef, type KeyboardEvent, - type AnchorHTMLAttributes, - type ButtonHTMLAttributes, + type HTMLAttributes, + forwardRef, + type RefObject, } from 'react'; import { useFloating, @@ -34,92 +35,26 @@ import { type Placement, type SizeOptions, } from '@floating-ui/react-dom'; -import type { IconComponentType } from '@sumup-oss/icons'; import type { ClickEvent } from '../../types/events.js'; -import type { EmotionAsPropType } from '../../types/prop-types.js'; import { isArrowDown, isArrowUp } from '../../util/key-codes.js'; import { isFunction } from '../../util/type-check.js'; import { clsx } from '../../styles/clsx.js'; import { useEscapeKey } from '../../hooks/useEscapeKey/index.js'; import { useClickOutside } from '../../hooks/useClickOutside/index.js'; import { useMedia } from '../../hooks/useMedia/index.js'; -import { useFocusList } from '../../hooks/useFocusList/index.js'; import { usePrevious } from '../../hooks/usePrevious/index.js'; -import { useStackContext } from '../StackContext/index.js'; -import { useComponents } from '../ComponentsContext/index.js'; -import { Portal } from '../Portal/index.js'; -import { Hr } from '../Hr/index.js'; -import { sharedClasses } from '../../styles/shared.js'; +import { Modal } from '../Modal/index.js'; +import { getKeyboardFocusableElements } from '../Modal/ModalService.js'; +import { applyMultipleRefs } from '../../util/refs.js'; +import dialogPolyfill from '../../vendor/dialog-polyfill/index.js'; import classes from './Popover.module.css'; -export interface BaseProps { - /** - * The Popover item label. - */ - children: string; - /** - * Function that's called when the item is clicked. - */ - onClick?: (event: ClickEvent) => void; - /** - * Display an icon in addition to the label. Designed for 24px icons from `@sumup-oss/icons`. - */ - icon?: IconComponentType; - /** - * Destructive variant, changes the color of label and icon from blue to red to signal to the user that the action - * is irreversible or otherwise dangerous. Interactive states are the same for destructive variant. - */ - destructive?: boolean; - /** - * Disabled variant. Visually and functionally disable the button. - */ - disabled?: boolean; -} - -type LinkElProps = Omit, 'onClick'>; -type ButtonElProps = Omit, 'onClick'>; - -export type PopoverItemProps = BaseProps & LinkElProps & ButtonElProps; - -export const PopoverItem = ({ - children, - icon: Icon, - destructive, - className, - ...props -}: PopoverItemProps) => { - const { Link } = useComponents(); - - const Element = props.href ? (Link as EmotionAsPropType) : 'button'; - - return ( - - {Icon && - ); -}; - -type Divider = { type: 'divider' }; -type Action = PopoverItemProps | Divider; - -function isDivider(action: Action): action is Divider { - return 'type' in action && action.type === 'divider'; -} - type OnToggle = (open: boolean | ((prevOpen: boolean) => boolean)) => void; -export interface PopoverProps { +export interface PopoverProps + extends Omit, 'children'> { /** * The class name to add to the Popover wrapper element. */ @@ -132,10 +67,6 @@ export interface PopoverProps { * Function that is called when opening and closing the Popover. */ onToggle: OnToggle; - /** - * An array of PopoverItem or Divider. - */ - actions: Action[]; /** * One of the accepted placement values. Defaults to `bottom`. */ @@ -164,15 +95,16 @@ export interface PopoverProps { 'aria-haspopup': boolean; 'aria-controls': string; 'aria-expanded': boolean; - }) => React.JSX.Element; + }) => ReactNode; /** - * Remove the [`menu` role](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/roles/menu_role) - * when its semantics aren't appropriate for the use case, for example when - * the Popover is used as part of a navigation. Default: 'menu'. - * - * Learn more: https://inclusive-components.design/menus-menu-buttons/ + * a ReactNode or a function that returns the content of the popover. + */ + children?: ReactNode | (() => ReactNode); + /** + * Text label for the close button for screen readers. + * Important for accessibility. */ - role?: 'menu' | null; + closeButtonLabel?: string; } type TriggerKey = 'ArrowUp' | 'ArrowDown'; @@ -186,192 +118,173 @@ const sizeOptions: SizeOptions = { }, }; -export const Popover = ({ - isOpen = false, - onToggle, - actions, - placement = 'bottom', - fallbackPlacements = ['top', 'right', 'left'], - component: Component, - offset, - className, - role = 'menu', - ...props -}: PopoverProps) => { - const zIndex = useStackContext(); - const triggerKey = useRef(null); - const menuEl = useRef(null); - const triggerId = useId(); - const menuId = useId(); - - const { x, y, strategy, refs, update } = useFloating({ - open: isOpen, - placement, - strategy: 'fixed', - middleware: offset - ? [ - offsetMiddleware(offset), - flip({ fallbackPlacements }), - size(sizeOptions), - ] - : [flip({ fallbackPlacements }), size(sizeOptions)], - }); - - const focusProps = useFocusList(); - const prevOpen = usePrevious(isOpen); - - const isMobile = useMedia('(max-width: 479px)'); - - const mobileStyles = { - position: 'fixed', - bottom: '0px', - left: '0px', - right: '0px', - width: 'auto', - zIndex: zIndex || 'var(--cui-z-index-popover)', - } as const; - - const handleToggle: OnToggle = useCallback( - (state) => { - onToggle((prev) => (isFunction(state) ? state(prev) : state)); +export const Popover = forwardRef( + ( + { + isOpen = false, + onToggle, + placement = 'bottom', + fallbackPlacements = ['top', 'right', 'left'], + component: Component, + offset, + className, + children, + closeButtonLabel, + ...props }, - [onToggle], - ); - - const handleTriggerClick = useCallback(() => { - handleToggle((prev) => !prev); - }, [handleToggle]); - - const handleTriggerKeyDown = useCallback( - (event: KeyboardEvent) => { - if (isArrowDown(event)) { - triggerKey.current = 'ArrowDown'; - handleToggle(true); - } - if (isArrowUp(event)) { - triggerKey.current = 'ArrowUp'; - handleToggle((prev) => !prev); - } - }, - [handleToggle], - ); - - const handlePopoverItemClick = - (onClick: BaseProps['onClick']) => (event: ClickEvent) => { - onClick?.(event); - handleToggle(false); - }; + ref, + ) => { + const triggerKey = useRef(null); + const menuEl = useRef(null); + const triggerRef = useRef(null); + const triggerId = useId(); + const menuId = useId(); + + const { floatingStyles, refs, update } = useFloating({ + open: isOpen, + placement, + strategy: 'fixed', + middleware: offset + ? [ + offsetMiddleware(offset), + flip({ fallbackPlacements, fallbackAxisSideDirection: 'start' }), + size(sizeOptions), + ] + : [flip({ fallbackPlacements }), size(sizeOptions)], + }); + + const prevOpen = usePrevious(isOpen); + + const isMobile = useMedia('(max-width: 479px)'); + + const handleToggle: OnToggle = useCallback( + (state) => { + onToggle((prev) => (isFunction(state) ? state(prev) : state)); + }, + [onToggle], + ); - useEscapeKey(() => handleToggle(false), isOpen); - useClickOutside( - [refs.reference, refs.floating], - () => handleToggle(false), - isOpen, - ); + const handleTriggerClick = useCallback(() => { + handleToggle((prev) => !prev); + }, [handleToggle]); + + const handleTriggerKeyDown = useCallback( + (event: KeyboardEvent) => { + if (isArrowDown(event)) { + triggerKey.current = 'ArrowDown'; + handleToggle(true); + } + if (isArrowUp(event)) { + triggerKey.current = 'ArrowUp'; + handleToggle((prev) => !prev); + } + }, + [handleToggle], + ); - useEffect(() => { - /** - * When we support `ResizeObserver` (https://caniuse.com/resizeobserver), - * we can look into using Floating UI's `autoUpdate` (but we can't use - * `whileElementIsMounted` because our implementation hides the floating - * element using CSS instead of using conditional rendering. - * See https://floating-ui.com/docs/react-dom#updating - */ - if (isOpen) { - update(); - window.addEventListener('resize', update); - window.addEventListener('scroll', update); - } else { - window.removeEventListener('resize', update); - window.removeEventListener('scroll', update); - } + useEscapeKey(() => handleToggle(false), isOpen); + useClickOutside( + [refs.reference as RefObject, refs.floating], + () => handleToggle(false), + isOpen, + ); - return () => { - window.removeEventListener('resize', update); - window.removeEventListener('scroll', update); - }; - }, [isOpen, update]); + useEffect(() => { + const dialogElement = refs.floating.current; - useEffect(() => { - // Focus the first or last popover item after opening - if (!prevOpen && isOpen) { - const element = ( - triggerKey.current && triggerKey.current === 'ArrowUp' - ? menuEl.current?.lastElementChild - : menuEl.current?.firstElementChild - ) as HTMLElement; - if (element) { - element.focus(); + if (!dialogElement) { + return; } - } - // Focus the reference element after closing - if (prevOpen && !isOpen) { - const triggerButton = refs.reference.current - ?.firstElementChild as HTMLElement; - triggerButton.focus(); - } + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore The package is bundled incorrectly + dialogPolyfill.registerDialog(dialogElement); + }, [refs.floating.current]); + + useEffect(() => { + /** + * When we support `ResizeObserver` (https://caniuse.com/resizeobserver), + * we can look into using Floating UI's `autoUpdate` (but we can't use + * `whileElementIsMounted` because our implementation hides the floating + * element using CSS instead of using conditional rendering. + * See https://floating-ui.com/docs/react-dom#updating + */ + if (isOpen) { + update(); + window.addEventListener('resize', update); + window.addEventListener('scroll', update); + } else { + window.removeEventListener('resize', update); + window.removeEventListener('scroll', update); + } - triggerKey.current = null; - }, [isOpen, prevOpen, refs.reference]); + return () => { + window.removeEventListener('resize', update); + window.removeEventListener('scroll', update); + }; + }, [isOpen, update]); + + useEffect(() => { + // Focus the first or last popover item after opening + if (!prevOpen && isOpen && !isMobile && menuEl.current) { + const elements = getKeyboardFocusableElements(menuEl.current); + if (elements.length) { + elements[0].focus(); + } + } - const isMenu = role === 'menu'; + // Focus the reference element after closing + if (prevOpen && !isOpen) { + const triggerButton = triggerRef.current + ?.firstElementChild as HTMLElement; + triggerButton.focus(); + } - return ( - -
- -
- -
-
-
- {actions.map((action, index) => - isDivider(action) ? ( -
- ) : ( - - ), - )} + triggerKey.current = null; + }, [isOpen, prevOpen, isMobile]); + + return ( + +
+
+
- - - ); -}; + {isMobile ? ( + + {children} + + ) : ( + +
+ {typeof children === 'function' ? children?.() : children} +
+ + )} + + ); + }, +); diff --git a/packages/circuit-ui/components/Popover/index.tsx b/packages/circuit-ui/components/Popover/index.tsx index 754ccc2aac..1b5ba597bb 100644 --- a/packages/circuit-ui/components/Popover/index.tsx +++ b/packages/circuit-ui/components/Popover/index.tsx @@ -15,4 +15,4 @@ export { Popover } from './Popover.js'; -export type { PopoverProps, PopoverItemProps } from './Popover.js'; +export type { PopoverProps } from './Popover.js'; diff --git a/packages/circuit-ui/index.ts b/packages/circuit-ui/index.ts index 738a98fa4d..cf046ffa60 100644 --- a/packages/circuit-ui/index.ts +++ b/packages/circuit-ui/index.ts @@ -151,10 +151,7 @@ export type { ProgressBarProps } from './components/ProgressBar/index.js'; export { Tag } from './components/Tag/index.js'; export type { TagProps } from './components/Tag/index.js'; export { Popover } from './components/Popover/index.js'; -export type { - PopoverProps, - PopoverItemProps, -} from './components/Popover/index.js'; +export type { PopoverProps } from './components/Popover/index.js'; export { ModalProvider } from './components/Modal/ModalContext.js'; export type { ModalProviderProps } from './components/Modal/ModalContext.js'; export { useModal } from './components/Modal/index.js';