Skip to content

Commit

Permalink
Refactor the Popover component (#2864)
Browse files Browse the repository at this point in the history 
* Refactor the Popover component using the Dialog component.

* Fix tests & snapshots

* remove wrapper

* fix trigger event issue

* fix trigger event issue

* use --cui-bg-elevated in all elevated components built with Dialog
  • Loading branch information
@sirineJ
sirineJ authored Jan 16, 2025
1 parent dee2aa9 commit 43013d2
Show file tree
Hide file tree
Showing 12 changed files with 486 additions and 451 deletions.
13 changes: 12 additions & 1 deletion packages/circuit-ui/components/Dialog/Dialog.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ export interface DialogProps
* @default `false`.
*/
initialFocusRef?: RefObject<HTMLElement>;
/**
* By passing a `preventOutsideClickRefs` ref or array of refs,
* you can prevent the dialog from closing when clicking on elements referenced by these refs.
*/
preventOutsideClickRefs?: RefObject<HTMLElement> | RefObject<HTMLElement>[];
/**
* A `ReactNode` or a function that returns the content of the modal dialog.
*/
Expand All @@ -109,6 +114,7 @@ export const Dialog = forwardRef<HTMLDialogElement, DialogProps>(
closeButtonLabel,
className,
initialFocusRef,
preventOutsideClickRefs,
preventClose = false,
animationDuration = 0,
onCloseStart,
Expand Down Expand Up @@ -259,7 +265,12 @@ export const Dialog = forwardRef<HTMLDialogElement, DialogProps>(
handleDialogClose();
}, [handleDialogClose]);

useClickOutside([dialogRef], handleOutsideClick, open && !isModal);
const useClickOutsideRefs = preventOutsideClickRefs
? // eslint-disable-next-line compat/compat
[dialogRef, preventOutsideClickRefs].flat()
: [dialogRef];

useClickOutside(useClickOutsideRefs, handleOutsideClick, open && !isModal);
useEscapeKey(() => handleDialogClose(), open && !isModal);

useEffect(() => {
Expand Down
8 changes: 4 additions & 4 deletions packages/circuit-ui/components/Dialog/dialog.module.css
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
.base {
padding: 0 !important;
pointer-events: none;
background-color: canvas;
background-color: var(--cui-bg-elevated);
border: none;
outline: none;
}
Expand Down Expand Up @@ -29,9 +29,9 @@
pointer-events: none;
content: "";
background: linear-gradient(
color-mix(in sRGB, var(--cui-bg-normal) 0%, transparent),
color-mix(in sRGB, var(--cui-bg-normal) 66%, transparent),
color-mix(in sRGB, var(--cui-bg-normal) 100%, transparent)
color-mix(in sRGB, var(--cui-bg-elevated) 0%, transparent),
color-mix(in sRGB, var(--cui-bg-elevated) 66%, transparent),
color-mix(in sRGB, var(--cui-bg-elevated) 100%, transparent)
);
border-radius: inherit;
}
Expand Down
8 changes: 0 additions & 8 deletions packages/circuit-ui/components/Modal/Modal.module.css
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,6 @@
background-color: var(--cui-bg-elevated);
}

.base::after {
background: linear-gradient(
color-mix(in sRGB, var(--cui-bg-elevated) 0%, transparent),
color-mix(in sRGB, var(--cui-bg-elevated) 66%, transparent),
color-mix(in sRGB, var(--cui-bg-elevated) 100%, transparent)
);
}

.content {
position: relative;
max-height: 90vh;
Expand Down
4 changes: 4 additions & 0 deletions packages/circuit-ui/components/Popover/Popover.mdx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ Popover menus are a common pattern to display a list of subsequent action option

<Story of={Stories.Offset} />

## Related components

This component is built on top of the low level [Dialog](Components/Dialog/Docs) component. If this component does not meet your requirements, you can use the Dialog component directly to build your own custom popover component.

## Usage guidelines

- **Do** use clear, concise and actionable labels for Popover items
Expand Down
100 changes: 23 additions & 77 deletions packages/circuit-ui/components/Popover/Popover.module.css
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,9 @@
.item {
display: flex;
align-items: center;
justify-content: flex-start;
width: 100%;
font-size: var(--cui-body-m-font-size);
line-height: var(--cui-body-m-line-height);
text-align: left;
background: var(--cui-bg-elevated);
}

.icon {
margin-right: var(--cui-spacings-kilo);
.base {
box-sizing: border-box;
max-height: var(--popover-max-height);
padding: 0;
margin: 0;
border-radius: var(--cui-border-radius-byte);
}

.trigger {
Expand All @@ -22,84 +15,37 @@
max-height: var(--popover-max-height);
padding: var(--cui-spacings-byte) 0;
overflow-y: auto;
visibility: hidden;
background-color: var(--cui-bg-elevated);
border: 1px solid var(--cui-border-subtle);
border-radius: var(--cui-border-radius-byte);
border: var(--cui-border-width-kilo) solid var(--cui-border-subtle);
border-radius: inherit;
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 {
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;
.base {
top: unset;
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);
width: auto;
max-width: 100%;
max-height: 90vh;
border-radius: var(--cui-border-radius-byte) var(--cui-border-radius-byte) 0
0;
}

.overlay.open {
visibility: inherit;
opacity: 1;
.menu {
max-height: 90vh;
padding: var(--cui-spacings-byte) 0;
}
}

.wrapper {
pointer-events: none;
}

.wrapper.open {
pointer-events: all;
.divider {
width: calc(100% - var(--cui-spacings-mega) * 2);
margin: var(--cui-spacings-byte) var(--cui-spacings-mega);
}

.wrapper.open::after {
position: absolute;
right: 0;
bottom: 0;
left: 0;
display: block;
.base::after {
height: var(--cui-spacings-kilo);
content: "";
background: linear-gradient(
color-mix(in sRGB, var(--cui-bg-elevated) 0%, transparent),
color-mix(in sRGB, var(--cui-bg-elevated) 66%, transparent),
color-mix(in sRGB, var(--cui-bg-elevated) 100%, transparent)
);
border-bottom-right-radius: var(--cui-border-radius-byte);
border-bottom-left-radius: var(--cui-border-radius-byte);
margin: var(--cui-border-width-kilo) var(--cui-border-width-mega);
}
102 changes: 28 additions & 74 deletions packages/circuit-ui/components/Popover/Popover.spec.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,81 +13,21 @@
* 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<T>(
renderFn: RenderFn<T>,
props: PopoverItemProps,
) {
return renderFn(<PopoverItem {...props} />);
}

const baseProps = {
children: 'PopoverItem',
icon: Download as FC<IconProps>,
};

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 { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import { createRef, type FC } from 'react';
import { Add, Delete, type IconProps } from '@sumup-oss/icons';
import { waitFor } from '@testing-library/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', () => {
beforeEach(() => {
vi.useFakeTimers({ shouldAdvanceTime: true });
});
afterEach(() => {
vi.useRealTimers();
vi.clearAllMocks();
});

Expand Down Expand Up @@ -134,6 +74,13 @@ describe('Popover', () => {
isOpen: true,
onToggle: vi.fn(createStateSetter(true)),
};
it('should forward a ref', () => {
const ref = createRef<HTMLDialogElement>();
render(<Popover {...baseProps} ref={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));
Expand Down Expand Up @@ -172,7 +119,9 @@ describe('Popover', () => {

await userEvent.click(document.body);

expect(baseProps.onToggle).toHaveBeenCalledTimes(1);
await waitFor(() => {
expect(baseProps.onToggle).toHaveBeenCalledTimes(1);
});
});

it('should close the popover when clicking the trigger element', async () => {
Expand All @@ -182,7 +131,8 @@ describe('Popover', () => {

await userEvent.click(popoverTrigger);

expect(baseProps.onToggle).toHaveBeenCalledTimes(1);
// TODO Find a better way to test this as toHaveBeenCalled is not reliable here.
expect(baseProps.onToggle).toHaveBeenCalled();
});

it.each([
Expand All @@ -193,6 +143,7 @@ describe('Popover', () => {
'should close the popover when pressing the %s key on the trigger element',
async (_, key) => {
renderPopover(baseProps);
vi.runAllTimers();

const popoverTrigger = screen.getByRole('button');

Expand All @@ -208,7 +159,7 @@ describe('Popover', () => {

await userEvent.keyboard('{Escape}');

expect(baseProps.onToggle).toHaveBeenCalledTimes(1);
await waitFor(() => expect(baseProps.onToggle).toHaveBeenCalledTimes(1));
});

it('should close the popover when clicking a popover item', async () => {
Expand Down Expand Up @@ -247,7 +198,9 @@ describe('Popover', () => {

const popoverTrigger = screen.getByRole('button');

expect(popoverTrigger).toHaveFocus();
await waitFor(() => {
expect(popoverTrigger).toHaveFocus();
});

await flushMicrotasks();
});
Expand Down Expand Up @@ -286,6 +239,7 @@ describe('Popover', () => {
it('should hide dividers from the accessibility tree', async () => {
const { baseElement } = renderPopover(baseProps);

// eslint-disable-next-line testing-library/no-node-access
const dividers = baseElement.querySelectorAll('hr[aria-hidden="true"');
expect(dividers.length).toBe(1);

Expand Down
Loading

0 comments on commit 43013d2

Please sign in to comment.