From 2c01edae7f6da9facfe3c8ba90ba86791ec43e34 Mon Sep 17 00:00:00 2001 From: sirineJ <112706079+sirineJ@users.noreply.github.com> Date: Wed, 18 Dec 2024 00:43:32 +0100 Subject: [PATCH] code review Pt 1 --- .../components/Modal/Modal.module.css | 9 ++-- .../components/Modal/Modal.spec.tsx | 53 +++++++++++-------- .../circuit-ui/components/Modal/Modal.tsx | 40 ++++++++------ .../components/Modal/ModalContext.spec.tsx | 3 -- 4 files changed, 57 insertions(+), 48 deletions(-) diff --git a/packages/circuit-ui/components/Modal/Modal.module.css b/packages/circuit-ui/components/Modal/Modal.module.css index 3c80765f61..790df57432 100644 --- a/packages/circuit-ui/components/Modal/Modal.module.css +++ b/packages/circuit-ui/components/Modal/Modal.module.css @@ -26,6 +26,7 @@ bottom: 0; left: 0; display: block; + height: var(--cui-spacings-giga); content: ""; background: linear-gradient( color-mix(in sRGB, var(--cui-bg-elevated) 0%, transparent), @@ -43,10 +44,6 @@ border-radius: var(--cui-border-radius-mega); } - .base::after { - height: var(--cui-spacings-giga); - } - .base .content { padding: var(--cui-spacings-giga); padding-bottom: calc( @@ -63,8 +60,8 @@ left: 0; width: 100%; max-width: 100%; - border-bottom-right-radius: 0; - border-bottom-left-radius: 0; + border-radius: var(--cui-border-radius-mega) var(--cui-border-radius-mega) 0 + 0; animation: slide-out 0.3s forwards; } diff --git a/packages/circuit-ui/components/Modal/Modal.spec.tsx b/packages/circuit-ui/components/Modal/Modal.spec.tsx index a038104044..6e8a97e22d 100644 --- a/packages/circuit-ui/components/Modal/Modal.spec.tsx +++ b/packages/circuit-ui/components/Modal/Modal.spec.tsx @@ -67,55 +67,64 @@ describe('Modal', () => { it('should forward a ref', () => { const ref = createRef(); - const { container } = render(); + render(); // eslint-disable-next-line testing-library/no-container - const dialog = container.querySelector('dialog'); + const dialog = screen.getByRole('dialog', { hidden: true }); expect(ref.current).toBe(dialog); }); it('should merge a custom class name with the default ones', () => { const className = 'foo'; - const { container } = render(); + render(); // eslint-disable-next-line testing-library/no-container - const dialog = container.querySelector('dialog'); + const dialog = screen.getByRole('dialog', { hidden: true }); expect(dialog?.className).toContain(className); }); it('should render in closed state by default', () => { - const { container } = render(); + render(); // eslint-disable-next-line testing-library/no-container - const dialog = container.querySelector('dialog') as HTMLDialogElement; + const dialog = screen.getByRole('dialog', { hidden: true }); expect(dialog).not.toBeVisible(); }); it('should open the dialog when the open prop becomes truthy', () => { - const { container, rerender } = render(); + const { rerender } = render(); // eslint-disable-next-line testing-library/no-container - const dialog = container.querySelector('dialog') as HTMLDialogElement; + const dialog = screen.getByRole('dialog', { + hidden: true, + }); vi.spyOn(dialog, 'showModal'); rerender(); expect(dialog.showModal).toHaveBeenCalledOnce(); + expect(dialog).toBeVisible(); }); it('should close the dialog when the open prop becomes falsy', () => { - const { container, rerender } = render(); + const { rerender } = render(); // eslint-disable-next-line testing-library/no-container - const dialog = container.querySelector('dialog') as HTMLDialogElement; + const dialog = screen.getByRole('dialog', { + hidden: true, + }); vi.spyOn(dialog, 'close'); rerender(); act(() => { vi.advanceTimersByTime(ANIMATION_DURATION); }); expect(dialog.close).toHaveBeenCalledOnce(); + expect(dialog).not.toBeVisible(); }); it('should close the dialog when the component is unmounted', async () => { - const { container, unmount } = render(); + const { unmount } = render(); // eslint-disable-next-line testing-library/no-container - const dialog = container.querySelector('dialog') as HTMLDialogElement; + const dialog = screen.getByRole('dialog', { + hidden: true, + }); vi.spyOn(dialog, 'close'); unmount(); expect(dialog.close).toHaveBeenCalledOnce(); + expect(dialog).not.toBeVisible(); }); describe('when the dialog is closed', () => { @@ -142,46 +151,43 @@ describe('Modal', () => { }); it('should not close modal on backdrop click if preventClose is true', async () => { - const { container } = render(); + render(); // eslint-disable-next-line testing-library/no-container - const dialog = container.querySelector('dialog') as HTMLDialogElement; + const dialog = screen.getByRole('dialog', { hidden: true }); await userEvent.click(dialog); act(() => { vi.advanceTimersByTime(ANIMATION_DURATION); }); expect(props.onClose).not.toHaveBeenCalled(); - }); - - it('should open in immersive mode', async () => { - const { container } = render( - , - ); - // eslint-disable-next-line testing-library/no-container - const dialog = container.querySelector('dialog') as HTMLDialogElement; - expect(dialog.className).toContain('immersive'); + expect(dialog).toBeVisible(); }); it('should close the dialog when pressing the backdrop', async () => { render(); + const dialog = screen.getByRole('dialog', { hidden: true }); await userEvent.click(screen.getByRole('dialog', { hidden: true })); act(() => { vi.advanceTimersByTime(ANIMATION_DURATION); }); expect(props.onClose).toHaveBeenCalledOnce(); + expect(dialog).not.toBeVisible(); }); it('should close the dialog when the close button is clicked', async () => { render(); + const dialog = screen.getByRole('dialog', { hidden: true }); await userEvent.click(screen.getByRole('button', { name: 'Close' })); act(() => { vi.advanceTimersByTime(ANIMATION_DURATION); }); expect(props.onClose).toHaveBeenCalledOnce(); + expect(dialog).not.toBeVisible(); }); it('should remove animation classes when closed when polyfill is used', async () => { (hasNativeDialogSupport as Mock).mockReturnValue(false); render(); + const dialog = screen.getByRole('dialog', { hidden: true }); const backdrop = document.getElementsByClassName('backdrop')[0]; expect(backdrop.classList.toString()).toContain('backdrop-visible'); @@ -192,6 +198,7 @@ describe('Modal', () => { }); expect(props.onClose).toHaveBeenCalledOnce(); + expect(dialog).not.toBeVisible(); }); }); diff --git a/packages/circuit-ui/components/Modal/Modal.tsx b/packages/circuit-ui/components/Modal/Modal.tsx index 56911da10f..7d1a72c360 100644 --- a/packages/circuit-ui/components/Modal/Modal.tsx +++ b/packages/circuit-ui/components/Modal/Modal.tsx @@ -1,6 +1,5 @@ -'use client'; /** - * Copyright 2024, SumUp Ltd. + * Copyright 2019, SumUp Ltd. * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at @@ -34,6 +33,7 @@ import type { ClickEvent } from '../../types/events.js'; import { isEscape } from '../../util/key-codes.js'; import { useI18n } from '../../hooks/useI18n/useI18n.js'; import { deprecate } from '../../util/logger.js'; +import type { Locale } from '../../util/i18n.js'; import classes from './Modal.module.css'; import { createUseModal } from './createUseModal.js'; @@ -79,10 +79,16 @@ export interface ModalProps * Enables focusing a particular element in the dialog content and override default behavior */ initialFocusRef?: RefObject; - /** - * @deprecated this props was passed to react-modal and is no longer relevant. - * Use preventClose instead. Also see https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/dialog_role#required_javascript_features + * One or more [IETF BCP 47](https://en.wikipedia.org/wiki/IETF_language_tag) + * locale identifiers such as `'de-DE'` or `['GB', 'en-US']`. + * When passing an array, the first supported locale is used. + * Defaults to `navigator.language` in supported environments. + */ + locale?: Locale; + /** + * @deprecated This prop was passed to `react-modal` and is no longer relevant. + * Use the `preventClose` prop instead. Also see https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/dialog_role#required_javascript_features */ hideCloseButton?: boolean; [key: DataAttribute]: string | undefined; @@ -94,6 +100,7 @@ export const Modal = forwardRef((props, ref) => { const { open, onClose, + locale, closeButtonLabel, variant = 'contextual', children, @@ -110,7 +117,7 @@ export const Modal = forwardRef((props, ref) => { if (hideCloseButton) { deprecate( 'Modal', - 'The "hideCloseButton" prop has been deprecated. Use the `preventClose` prop instead.', + 'The `hideCloseButton` prop has been deprecated. Use the `preventClose` prop instead.', ); } } @@ -118,7 +125,6 @@ export const Modal = forwardRef((props, ref) => { const hasNativeDialog = hasNativeDialogSupport(); // set initial focus on the modal dialog content - // biome-ignore lint/correctness/useExhaustiveDependencies: we only want to run this effect once useEffect(() => { const dialogElement = dialogRef.current; let timeoutId: NodeJS.Timeout; @@ -134,21 +140,20 @@ export const Modal = forwardRef((props, ref) => { return () => { clearTimeout(timeoutId); }; - }, [open]); - - const setScrollProperty = useCallback(() => { - document.documentElement.style.setProperty( - '--scroll-y', - `${window.scrollY}px`, - ); - }, []); + }, [open, initialFocusRef?.current]); useEffect(() => { + function setScrollProperty() { + document.documentElement.style.setProperty( + '--scroll-y', + `${window.scrollY}px`, + ); + } window.addEventListener('scroll', setScrollProperty); return () => { window.removeEventListener('scroll', setScrollProperty); }; - }, [setScrollProperty]); + }, []); const handleDialogClose = useCallback(() => { const dialogElement = dialogRef.current; @@ -288,6 +293,9 @@ export const Modal = forwardRef((props, ref) => { const onDialogClick = ( event: ClickEvent | ClickEvent, ) => { + // the dialog content covers the whole dialog element + // leaving the backdrop element as the only clickable area + // that can trigger an onClick event if (event.target === event.currentTarget && !preventClose) { handleDialogClose(); } diff --git a/packages/circuit-ui/components/Modal/ModalContext.spec.tsx b/packages/circuit-ui/components/Modal/ModalContext.spec.tsx index 5cb11ecdc6..866ea84db5 100644 --- a/packages/circuit-ui/components/Modal/ModalContext.spec.tsx +++ b/packages/circuit-ui/components/Modal/ModalContext.spec.tsx @@ -61,9 +61,6 @@ describe('ModalContext', () => { vi.advanceTimersByTime(ANIMATION_DURATION); }); - expect( - (screen.getByTestId('dummy-dialog')).open, - ).toBe(true); expect(screen.getByTestId('dummy-dialog')).toBeVisible(); });