code review

sumup-oss · Jan 23, 2025 · 6b3c420 · 6b3c420
1 parent cc0049e
commit 6b3c420
Show file tree

Hide file tree

Showing 9 changed files with 108 additions and 89 deletions.
diff --git a/packages/circuit-ui/components/DateInput/DateInput.stories.tsx b/packages/circuit-ui/components/DateInput/DateInput.stories.tsx
@@ -48,7 +48,7 @@ const openCalendar = async ({
 }) => {
   const canvas = within(canvasElement);
   const button = canvas.getByRole('button', {
-    name: 'Change date',
+    name: 'Change date, August 28, 2017',
   });
 
   await userEvent.click(button);
@@ -114,6 +114,7 @@ export const Optional = (args: DateInputProps) => <DateInput {...args} />;
 Optional.args = {
   ...baseArgs,
   optionalLabel: 'optional',
+  defaultValue: '2017-08-28',
 };
 Optional.play = openCalendar;
 

diff --git a/packages/circuit-ui/components/DateInput/DateInput.tsx b/packages/circuit-ui/components/DateInput/DateInput.tsx
@@ -336,12 +336,12 @@ export const DateInput = forwardRef<HTMLInputElement, DateInputProps>(
     }
     const [isClosing, setIsClosing] = useState(false);
 
-    const handleModalCloseEnd = useCallback(() => {
+    const handleDialogCloseEnd = useCallback(() => {
       setIsClosing(false);
       closeCalendar?.();
     }, [closeCalendar]);
 
-    const handleModalCloseStart = useCallback(() => {
+    const handleDialogCloseStart = useCallback(() => {
       setIsClosing(true);
     }, []);
 
@@ -482,8 +482,8 @@ export const DateInput = forwardRef<HTMLInputElement, DateInputProps>(
           open={open}
           isModal={isMobile}
           hideCloseButton={!isMobile}
-          onCloseStart={handleModalCloseStart}
-          onCloseEnd={handleModalCloseEnd}
+          onCloseStart={handleDialogCloseStart}
+          onCloseEnd={handleDialogCloseEnd}
           className={clsx(
             classes.dialog,
             isClosing ? outAnimation : inAnimation,

diff --git a/packages/circuit-ui/components/Dialog/Dialog.tsx b/packages/circuit-ui/components/Dialog/Dialog.tsx
@@ -316,6 +316,24 @@ export const Dialog = forwardRef<HTMLDialogElement, DialogProps>(
     );
 
     // Focus Management
+    useEffect(() => {
+      // save the opening element to restore focus after the dialog closes
+      if (open) {
+        if (document.activeElement instanceof HTMLElement) {
+          lastFocusedElementRef.current = document.activeElement;
+        }
+      }
+      return () => {
+        // restore focus to the opening element
+        if (lastFocusedElementRef.current) {
+          setTimeout(
+            () => lastFocusedElementRef.current?.focus(),
+            animationDurationRef.current,
+          );
+        }
+      };
+    }, [open, animationDurationRef]);
+
     useEffect(() => {
       const dialogElement = dialogRef.current;
       let timeoutId: NodeJS.Timeout;
@@ -341,24 +359,6 @@ export const Dialog = forwardRef<HTMLDialogElement, DialogProps>(
       };
     }, [open, initialFocusRef, hideCloseButton, animationDurationRef.current]);
 
-    useEffect(() => {
-      // save the opening element to restore focus after the dialog closes
-      if (open) {
-        if (document.activeElement instanceof HTMLElement) {
-          lastFocusedElementRef.current = document.activeElement;
-        }
-      }
-      return () => {
-        // restore focus to the opening element
-        if (lastFocusedElementRef.current) {
-          setTimeout(
-            () => lastFocusedElementRef.current?.focus(),
-            animationDurationRef.current,
-          );
-        }
-      };
-    }, [open, animationDurationRef]);
-
     useEffect(() => {
       dialogRef.current?.style.setProperty(
         '--dialog-animation-duration',

diff --git a/...ircuit-ui/components/SideNavigation/components/MobileNavigation/MobileNavigation.spec.tsx b/...ircuit-ui/components/SideNavigation/components/MobileNavigation/MobileNavigation.spec.tsx
@@ -41,7 +41,7 @@ describe('MobileNavigation', () => {
 
   const baseProps = {
     open: true,
-    onCloseEnd: vi.fn(),
+    onClose: vi.fn(),
     closeButtonLabel: 'Close navigation modal',
     primaryNavigationLabel: 'Primary',
   };
@@ -128,7 +128,7 @@ describe('MobileNavigation', () => {
 
     await userEvent.click(primaryLinkEl);
 
-    expect(baseProps.onCloseEnd).toHaveBeenCalledTimes(1);
+    expect(baseProps.onClose).toHaveBeenCalledTimes(1);
     expect(onClick).toHaveBeenCalledTimes(1);
   });
 
@@ -177,7 +177,7 @@ describe('MobileNavigation', () => {
 
     await userEvent.click(secondaryLinkEl);
 
-    expect(baseProps.onCloseEnd).toHaveBeenCalledTimes(1);
+    expect(baseProps.onClose).toHaveBeenCalledTimes(1);
     expect(onClick).toHaveBeenCalledTimes(1);
   });
 

diff --git a/packages/circuit-ui/components/SidePanel/SidePanel.module.css b/packages/circuit-ui/components/SidePanel/SidePanel.module.css
@@ -6,6 +6,19 @@
   box-shadow: none;
 }
 
+.wrapper {
+  display: flex;
+  flex-direction: column;
+  align-items: center;
+  width: 100%;
+  height: 100%;
+  padding: 0;
+  padding: env(safe-area-inset-top) env(safe-area-inset-right)
+    env(safe-area-inset-bottom) env(safe-area-inset-left);
+  overflow-y: auto;
+  -webkit-overflow-scrolling: touch;
+}
+
 @media (max-width: 767px) {
   .base {
     top: 0;
@@ -35,6 +48,10 @@
     margin-left: 1px;
   }
 
+  .wrapper {
+    padding-left: 0;
+  }
+
   .content {
     padding: 0 var(--cui-spacings-giga);
   }

diff --git a/packages/circuit-ui/components/SidePanel/SidePanel.spec.tsx b/packages/circuit-ui/components/SidePanel/SidePanel.spec.tsx
@@ -98,6 +98,16 @@ describe('SidePanel', () => {
     await waitFor(() => expect(onClose).toHaveBeenCalled());
   });
 
+  describe('when the panel is not stacked', () => {
+    it('should not show the back button', () => {
+      renderComponent();
+
+      expect(
+        screen.queryByTitle(baseProps.backButtonLabel as string),
+      ).toBeNull();
+    });
+  });
+
   describe('when the panel is stacked', () => {
     it('should show the back button', () => {
       const onBack = vi.fn();
@@ -145,6 +155,16 @@ describe('SidePanel', () => {
     });
   });
 
+  describe('when the panel is on desktop resolution', () => {
+    it('should describe the side panel as modal', () => {
+      const { rerender } = render(<SidePanel {...baseProps} open={false} />);
+      const dialog = screen.getByRole('dialog', { hidden: true });
+      vi.spyOn(dialog, 'show');
+      rerender(<SidePanel {...baseProps} open />);
+      expect(dialog.show).toHaveBeenCalledOnce();
+    });
+  });
+
   describe('when the panel is on mobile resolution', () => {
     it('should describe the side panel as modal', () => {
       const { rerender } = render(

diff --git a/packages/circuit-ui/components/SidePanel/SidePanel.tsx b/packages/circuit-ui/components/SidePanel/SidePanel.tsx
@@ -31,7 +31,6 @@ import { clsx } from '../../styles/clsx.js';
 import { useAnimation } from '../../hooks/useAnimation/index.js';
 import { sharedClasses } from '../../styles/shared.js';
 import { useI18n } from '../../hooks/useI18n/useI18n.js';
-import { useLatest } from '../../hooks/useLatest/index.js';
 
 import { Header } from './components/Header/index.js';
 import type { ChildrenRenderProps, OnBack, OnClose } from './useSidePanel.js';
@@ -94,29 +93,33 @@ export const SidePanel = forwardRef<HTMLDialogElement, SidePanelProps>(
 
       const [, setAnimating] = useAnimation();
 
-      const triggerAnimation = useCallback(
-        (isOpening: boolean, callBack?: OnBack | OnClose) => {
+      const onOpening = useCallback(() => {
+        setAnimating({
+          duration: animationDuration,
+          onStart: () => {
+            setAnimationClass(
+              animationDuration > 0
+                ? sharedClasses.animationSlideRightIn
+                : undefined,
+            );
+          },
+          onEnd: () => {
+            setAnimationClass(undefined);
+          },
+        });
+      }, [animationDuration, setAnimating]);
+
+      const onClosing = useCallback(
+        (callBack?: OnBack | OnClose) => {
           setAnimating({
             duration: animationDuration,
             onStart: () => {
-              if (isOpening) {
-                setAnimationClass(
-                  animationDuration > 0
-                    ? sharedClasses.animationSlideRightIn
-                    : undefined,
-                );
-              } else {
-                void callBack?.();
-                setAnimationClass(sharedClasses.animationSlideRightOut);
-              }
+              void callBack?.();
+              setAnimationClass(sharedClasses.animationSlideRightOut);
             },
             onEnd: () => {
-              if (isOpening) {
-                setAnimationClass(undefined);
-              } else {
-                setAnimationClass(classes.closed);
-                onCloseEnd?.();
-              }
+              setAnimationClass(classes.closed);
+              onCloseEnd?.();
             },
           });
         },
@@ -132,12 +135,10 @@ export const SidePanel = forwardRef<HTMLDialogElement, SidePanelProps>(
         setHeaderSticky(event.currentTarget.scrollTop > 0);
       };
 
-      const triggerAnimationRef = useLatest(triggerAnimation);
-
       useEffect(() => {
         // trigger animation on opening
-        triggerAnimationRef.current(true);
-      }, [triggerAnimationRef]);
+        onOpening();
+      }, [onOpening]);
 
       useEffect(
         () => () => {
@@ -146,23 +147,6 @@ export const SidePanel = forwardRef<HTMLDialogElement, SidePanelProps>(
         [onCloseEnd],
       );
 
-      const content = (
-        <div onScroll={handleScroll}>
-          <Header
-            backButtonLabel={backButtonLabel}
-            closeButtonLabel={closeButtonLabel}
-            headline={headline}
-            id={headerAriaId}
-            onBack={onBack ? () => triggerAnimation(false, onBack) : undefined}
-            onClose={() => triggerAnimation(false, onClose)}
-            isSticky={isHeaderSticky}
-          />
-          <div className={classes.content}>
-            {isFunction(children) ? children({ onBack, onClose }) : children}
-          </div>
-        </div>
-      );
-
       return (
         <Dialog
           {...rest}
@@ -172,12 +156,25 @@ export const SidePanel = forwardRef<HTMLDialogElement, SidePanelProps>(
           aria-labelledby={headerAriaId}
           animationDuration={animationDuration}
           className={clsx(classes.base, animationClass, className)}
-          onCloseStart={() => triggerAnimation(false, onBack || onClose)}
+          onCloseStart={() => onClosing(onBack || onClose)}
           preventOutsideClickClose={true}
           preventEscapeKeyClose={preventEscapeKeyClose}
           hideCloseButton
         >
-          {content}
+          <div className={classes.wrapper} onScroll={handleScroll}>
+            <Header
+              backButtonLabel={backButtonLabel}
+              closeButtonLabel={closeButtonLabel}
+              headline={headline}
+              id={headerAriaId}
+              onBack={onBack ? () => onClosing(onBack) : undefined}
+              onClose={() => onClosing(onClose)}
+              isSticky={isHeaderSticky}
+            />
+            <div className={classes.content}>
+              {isFunction(children) ? children({ onBack, onClose }) : children}
+            </div>
+          </div>
         </Dialog>
       );
     }

diff --git a/packages/circuit-ui/components/SidePanel/SidePanelContext.spec.tsx b/packages/circuit-ui/components/SidePanel/SidePanelContext.spec.tsx
@@ -24,7 +24,7 @@ import {
   vi,
   type Mock,
 } from 'vitest';
-import { useContext, type ComponentType, useEffect } from 'react';
+import { useContext, type ComponentType } from 'react';
 
 import {
   render,
@@ -48,26 +48,6 @@ import {
 
 vi.mock('../../hooks/useMedia/index.js');
 
-vi.mock('./SidePanel.js', () => ({
-  SidePanel: ({
-    onCloseEnd,
-    children,
-  }: Pick<SidePanelContextItem, 'onCloseEnd' | 'children'>) => {
-    useEffect(
-      () => () => {
-        onCloseEnd?.();
-      },
-      [onCloseEnd],
-    );
-
-    return (
-      <dialog open={true}>
-        {typeof children === 'function' ? children({}) : children}
-      </dialog>
-    );
-  },
-}));
-
 describe('SidePanelContext', () => {
   beforeAll(() => {
     vi.useFakeTimers();

diff --git a/packages/circuit-ui/components/SidePanel/SidePanelContext.tsx b/packages/circuit-ui/components/SidePanel/SidePanelContext.tsx
@@ -142,6 +142,10 @@ export function SidePanelProvider({
               resolve();
             }
 
+            dispatch({
+              type: 'update',
+              item: sidePanel,
+            });
             dispatch({
               type: 'remove',
               id: sidePanel.id,