code review Pt 1

sumup-oss · Dec 18, 2024 · 3e9df87 · 3e9df87
1 parent 972eb20
commit 3e9df87
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 52 deletions.
diff --git a/packages/circuit-ui/components/Modal/Modal.module.css b/packages/circuit-ui/components/Modal/Modal.module.css
@@ -48,10 +48,8 @@
   }
 
   .base .content {
-    padding: var(--cui-spacings-giga);
-    padding-bottom: calc(
-      env(safe-area-inset-bottom) + var(--cui-spacings-giga)
-    );
+    padding: var(--cui-spacings-giga) var(--cui-spacings-giga) 0;
+    padding-bottom: env(safe-area-inset-bottom);
   }
 }
 
@@ -63,8 +61,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;
   }
 
@@ -79,11 +77,13 @@
     border-radius: unset;
   }
 
+  .base::after {
+    height: var(--cui-spacings-mega);
+  }
+
   .base .content {
-    padding: var(--cui-spacings-mega);
-    padding-bottom: calc(
-      env(safe-area-inset-bottom) + var(--cui-spacings-mega)
-    );
+    padding: var(--cui-spacings-mega) var(--cui-spacings-mega) 0;
+    padding-bottom: env(safe-area-inset-bottom);
     -webkit-overflow-scrolling: touch;
   }
 }

diff --git 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<HTMLDialogElement>();
-    const { container } = render(<Modal {...props} ref={ref} />);
+    render(<Modal {...props} ref={ref} />);
     // 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(<Modal {...props} className={className} />);
+    render(<Modal {...props} className={className} />);
     // 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(<Modal {...props} />);
+    render(<Modal {...props} />);
     // 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(<Modal {...props} />);
+    const { rerender } = render(<Modal {...props} />);
     // 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(<Modal {...props} open />);
     expect(dialog.showModal).toHaveBeenCalledOnce();
+    expect(dialog).toBeVisible();
   });
 
   it('should close the dialog when the open prop becomes falsy', () => {
-    const { container, rerender } = render(<Modal {...props} open />);
+    const { rerender } = render(<Modal {...props} open />);
     // 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(<Modal {...props} />);
     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(<Modal {...props} open />);
+    const { unmount } = render(<Modal {...props} open />);
     // 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(<Modal {...props} open preventClose />);
+      render(<Modal {...props} open preventClose />);
       // 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(
-        <Modal {...props} open variant="immersive" />,
-      );
-      // 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(<Modal {...props} open />);
+      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(<Modal {...props} open />);
+      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(<Modal {...props} open />);
+      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
@@ -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<HTMLElement>;
-
   /**
-   * @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<HTMLDialogElement, ModalProps>((props, ref) => {
   const {
     open,
     onClose,
+    locale,
     closeButtonLabel,
     variant = 'contextual',
     children,
@@ -110,15 +117,14 @@ export const Modal = forwardRef<HTMLDialogElement, ModalProps>((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.',
       );
     }
   }
 
   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<HTMLDialogElement, ModalProps>((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<HTMLDialogElement, ModalProps>((props, ref) => {
   const onDialogClick = (
     event: ClickEvent<HTMLDialogElement> | ClickEvent<HTMLDivElement>,
   ) => {
+    // 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
@@ -61,9 +61,6 @@ describe('ModalContext', () => {
         vi.advanceTimersByTime(ANIMATION_DURATION);
       });
 
-      expect(
-        (screen.getByTestId('dummy-dialog')).open,
-      ).toBe(true);
       expect(screen.getByTestId('dummy-dialog')).toBeVisible();
     });