Refactor / replace modal with HTML Dialog (#505)

* refactor: use html dialog elements * refactor(player): fix endless unmount loop * refactor: re-fix backdrop click to close modal * refactor(project): use events for animation open close callbacks * test(e2e): fix close modal by backdrop test * test(e2e): fix timeout on choose offer modal * test(e2e): skip click outside modal test for mobile --------- Co-authored-by: Roy Schut <roy@videodock.com>
jwplayer · May 13, 2024 · 6ee0305 · 6ee0305
1 parent 065e9c1
commit 6ee0305
Show file tree

Hide file tree

Showing 31 changed files with 520 additions and 577 deletions.
diff --git a/packages/hooks-react/src/useOffers.ts b/packages/hooks-react/src/useOffers.ts
@@ -1,5 +1,4 @@
-import { useMutation } from 'react-query';
-import { useEffect } from 'react';
+import { useMutation, useQuery } from 'react-query';
 import { shallow } from '@jwp/ott-common/src/utils/compare';
 import { getModule } from '@jwp/ott-common/src/modules/container';
 import { useCheckoutStore } from '@jwp/ott-common/src/stores/CheckoutStore';
@@ -21,9 +20,9 @@ const useOffers = () => {
     shallow,
   );
 
-  const { mutate: initialise, isLoading: isInitialisationLoading } = useMutation<void>({
-    mutationKey: ['initialiseOffers', requestedMediaOffers],
-    mutationFn: checkoutController.initialiseOffers,
+  const { isLoading: isInitialisationLoading } = useQuery<void>({
+    queryKey: ['initialiseOffers', requestedMediaOffers],
+    queryFn: checkoutController.initialiseOffers,
   });
 
   const chooseOffer = useMutation({
@@ -37,10 +36,6 @@ const useOffers = () => {
     onSuccess: () => accountController.reloadSubscriptions({ delay: 3000, retry: 10 }), // A subscription switch usually takes at least 3 secs
   });
 
-  useEffect(() => {
-    initialise();
-  }, [requestedMediaOffers, initialise]);
-
   const hasMediaOffers = mediaOffers.length > 0;
   const hasSubscriptionOffers = subscriptionOffers.length > 0;
   const hasPremierOffers = requestedMediaOffers.some((mediaOffer) => mediaOffer.premier);

diff --git a/packages/ui-react/package.json b/packages/ui-react/package.json
@@ -45,7 +45,8 @@
     "typescript-plugin-css-modules": "^5.0.2",
     "vi-fetch": "^0.8.0",
     "vite-plugin-svgr": "^4.2.0",
-    "vitest": "^1.3.1"
+    "vitest": "^1.3.1",
+    "wicg-inert": "^3.1.2"
   },
   "peerDependencies": {
     "@jwp/ott-common": "*",

diff --git a/packages/ui-react/src/components/Alert/Alert.test.tsx b/packages/ui-react/src/components/Alert/Alert.test.tsx
@@ -1,11 +1,12 @@
 import React from 'react';
-import { render } from '@testing-library/react';
+
+import { renderWithRouter } from '../../../test/utils';
 
 import Alert from './Alert';
 
 describe('<Alert>', () => {
   test('renders and matches snapshot', () => {
-    const { container } = render(<Alert message="Body" open={true} onClose={vi.fn()} />);
+    const { container } = renderWithRouter(<Alert message="Body" open={true} onClose={vi.fn()} />);
     expect(container).toMatchSnapshot();
   });
 });
diff --git a/packages/ui-react/src/components/Animation/Animation.tsx b/packages/ui-react/src/components/Animation/Animation.tsx
@@ -1,4 +1,5 @@
 import React, { type CSSProperties, useEffect, useRef, useState } from 'react';
+import useEventCallback from '@jwp/ott-hooks-react/src/useEventCallback';
 
 type Props = {
   className?: string;
@@ -27,35 +28,42 @@ const Animation: React.FC<Props> = ({
 }) => {
   const [status, setStatus] = useState<Status>('closed');
   const [hasOpenedBefore, setHasOpenedBefore] = useState<boolean>(false);
-  const seconds = duration / 1000;
-  const transition = `transform ${seconds}s ease-out`; // todo: -webkit-transform;
 
   const timeout = useRef<number>();
   const timeout2 = useRef<number>();
 
-  useEffect(() => {
-    if (timeout.current) clearTimeout(timeout.current);
-    if (timeout2.current) clearTimeout(timeout2.current);
-    if (open) {
-      setHasOpenedBefore(true);
-      timeout.current = window.setTimeout(() => setStatus('opening'), delay);
-      timeout2.current = window.setTimeout(() => {
-        setStatus('open');
-        onOpenAnimationEnd && onOpenAnimationEnd();
-      }, duration + delay);
-    } else if (hasOpenedBefore) {
+  // use event callbacks to ignore reactive dependencies
+  const openEvent = useEventCallback(() => {
+    setHasOpenedBefore(true);
+    timeout.current = window.setTimeout(() => setStatus('opening'), delay);
+    timeout2.current = window.setTimeout(() => {
+      setStatus('open');
+      onOpenAnimationEnd && onOpenAnimationEnd();
+    }, duration + delay);
+  });
+
+  const closeEvent = useEventCallback(() => {
+    if (hasOpenedBefore) {
       timeout.current = window.setTimeout(() => setStatus('closing'), delay);
       timeout2.current = window.setTimeout(() => {
         setStatus('closed');
         onCloseAnimationEnd && onCloseAnimationEnd();
       }, duration + delay);
     }
+  });
+
+  useEffect(() => {
+    if (open) {
+      openEvent();
+    } else {
+      closeEvent();
+    }
 
     return () => {
       clearTimeout(timeout.current);
       clearTimeout(timeout2.current);
     };
-  }, [duration, delay, transition, open, onOpenAnimationEnd, onCloseAnimationEnd, hasOpenedBefore, setHasOpenedBefore]);
+  }, [open, openEvent, closeEvent]);
 
   if (!open && status === 'closed' && !keepMounted) {
     return null;

diff --git a/packages/ui-react/src/components/ChooseOfferForm/ChooseOfferForm.test.tsx b/packages/ui-react/src/components/ChooseOfferForm/ChooseOfferForm.test.tsx
@@ -107,7 +107,7 @@ describe('<OffersForm>', () => {
 
     fireEvent.click(getByTestId('S345569153_NL'));
 
-    expect(onChange).toBeCalled();
+    expect(onChange).toHaveBeenCalled();
   });
 
   test('calls the onSubmit callback when submitting the form', () => {

diff --git a/packages/ui-react/src/components/ConfirmationDialog/ConfirmationDialog.test.tsx b/packages/ui-react/src/components/ConfirmationDialog/ConfirmationDialog.test.tsx
@@ -1,11 +1,12 @@
 import React from 'react';
-import { render } from '@testing-library/react';
+
+import { renderWithRouter } from '../../../test/utils';
 
 import ConfirmationDialog from './ConfirmationDialog';
 
 describe('<ConfirmationDialog>', () => {
   test('renders and matches snapshot', () => {
-    const { container } = render(<ConfirmationDialog body="Body" title="Title" open={true} onConfirm={vi.fn()} onClose={vi.fn()} />);
+    const { container } = renderWithRouter(<ConfirmationDialog body="Body" title="Title" open={true} onConfirm={vi.fn()} onClose={vi.fn()} />);
 
     expect(container).toMatchSnapshot();
   });

diff --git a/packages/ui-react/src/components/Dialog/Dialog.test.tsx b/packages/ui-react/src/components/Dialog/Dialog.test.tsx
@@ -1,14 +1,15 @@
 import React from 'react';
-import { render } from '@testing-library/react';
+
+import { renderWithRouter } from '../../../test/utils';
 
 import Dialog from './Dialog';
 
 describe('<Dialog>', () => {
   test('renders and matches snapshot', () => {
-    const { baseElement } = render(
+    const { baseElement } = renderWithRouter(
       <>
         <span>Some content</span>
-        <Dialog onClose={vi.fn()} open={true} role="dialog">
+        <Dialog onClose={vi.fn()} open={true}>
           Dialog contents
         </Dialog>
         <span>Some other content</span>
@@ -17,21 +18,4 @@ describe('<Dialog>', () => {
 
     expect(baseElement).toMatchSnapshot();
   });
-
-  test('Should ensure Dialog is properly marked as a modal and has role "dialog"', () => {
-    const { getByTestId } = render(
-      <>
-        <span>Some content</span>
-        <Dialog onClose={vi.fn()} open={true} role="dialog" data-testid="dialog">
-          Dialog contents
-        </Dialog>
-        <span>Some other content</span>
-      </>,
-    );
-
-    const dialogElement = getByTestId('dialog');
-
-    expect(dialogElement).toHaveAttribute('aria-modal', 'true');
-    expect(dialogElement).toHaveAttribute('role', 'dialog');
-  });
 });
diff --git a/packages/ui-react/src/components/Dialog/Dialog.tsx b/packages/ui-react/src/components/Dialog/Dialog.tsx
@@ -13,13 +13,13 @@ type Props = {
   onClose: () => void;
   size?: 'small' | 'large';
   children: React.ReactNode;
-  role: React.AriaRole;
+  role?: React.AriaRole;
 } & React.AriaAttributes;
 
-const Dialog: React.FC<Props> = ({ open, onClose, children, size = 'small', role = 'dialog', ...ariaAttributes }: Props) => {
+const Dialog: React.FC<Props> = ({ open, onClose, children, size = 'small', role, ...ariaAttributes }: Props) => {
   return (
-    <Modal open={open} onClose={onClose} AnimationComponent={Slide}>
-      <div className={classNames(styles.dialog, styles[size])} aria-modal="true" role={role} data-testid={testId('dialog')} {...ariaAttributes}>
+    <Modal open={open} onClose={onClose} AnimationComponent={Slide} role={role} centered>
+      <div className={classNames(styles.dialog, styles[size])} data-testid={testId('dialog')} {...ariaAttributes}>
         {children}
         <ModalCloseButton onClick={onClose} />
       </div>

diff --git a/packages/ui-react/src/components/Dialog/__snapshots__/Dialog.test.tsx.snap b/packages/ui-react/src/components/Dialog/__snapshots__/Dialog.test.tsx.snap
@@ -12,53 +12,40 @@ exports[`<Dialog> > renders and matches snapshot 1`] = `
       Some other content
     </span>
   </div>
-  <div
-    style="transition: opacity 0.3s ease-in-out; opacity: 0; will-change: opacity;"
+  <dialog
+    class="_centered_6c6c55"
+    open=""
   >
     <div
-      class="_modal_6c6c55"
+      style="transition: transform 0.3s ease, opacity 0.3s ease; transform: translate(0, -15px); opacity: 0; z-index: 15;"
     >
       <div
-        class="_backdrop_6c6c55"
-        data-testid="backdrop"
-      />
-      <div
-        class="_container_6c6c55"
+        class="_dialog_00d559 _small_00d559"
+        data-testid="dialog"
       >
-        <div
-          style="transition: transform 0.2s ease, opacity 0.2s ease; transform: translate(0, -15px); opacity: 0; z-index: 15;"
+        Dialog contents
+        <button
+          aria-label="close_modal"
+          class="_iconButton_0fef65 _modalCloseButton_b92d69"
+          type="button"
         >
-          <div
-            aria-modal="true"
-            class="_dialog_00d559 _small_00d559"
-            data-testid="dialog"
-            role="dialog"
+          <svg
+            aria-hidden="true"
+            class="_icon_585b29"
+            viewBox="0 0 24 24"
+            xmlns="http://www.w3.org/2000/svg"
           >
-            Dialog contents
-            <button
-              aria-label="close_modal"
-              class="_iconButton_0fef65 _modalCloseButton_b92d69"
-              type="button"
-            >
-              <svg
-                aria-hidden="true"
-                class="_icon_585b29"
-                viewBox="0 0 24 24"
-                xmlns="http://www.w3.org/2000/svg"
-              >
-                <path
-                  d="M19 6.41L17.59 5 12 10.59 6.41 5 5 6.41 10.59 12 5 17.59 6.41 19 12 13.41 17.59 19 19 17.59 13.41 12z"
-                />
-                <path
-                  d="M0 0h24v24H0z"
-                  fill="none"
-                />
-              </svg>
-            </button>
-          </div>
-        </div>
+            <path
+              d="M19 6.41L17.59 5 12 10.59 6.41 5 5 6.41 10.59 12 5 17.59 6.41 19 12 13.41 17.59 19 19 17.59 13.41 12z"
+            />
+            <path
+              d="M0 0h24v24H0z"
+              fill="none"
+            />
+          </svg>
+        </button>
       </div>
     </div>
-  </div>
+  </dialog>
 </body>
 `;
diff --git a/packages/ui-react/src/components/Modal/Modal.module.scss b/packages/ui-react/src/components/Modal/Modal.module.scss
@@ -2,27 +2,64 @@
 @use '@jwp/ott-ui-react/src/styles/theme';
 @use '@jwp/ott-ui-react/src/styles/mixins/responsive';
 
-.modal {
-  position: fixed;
+$animation-duration: 0.3s;
+
+dialog {
   top: 0;
   left: 0;
-  z-index: variables.$modals-z-index;
   width: 100vw;
-  height: 100dvh;
+  max-width: 100vw;
+  height: 100%;
+  max-height: 100vh;
+  margin: 0;
+  padding: 0;
+  overflow: hidden;
+  color: var(--body-color);
+  background: transparent;
+  border: none;
+
+  &.centered[open] {
+    display: flex;
+    justify-content: center;
+    align-items: center;
+    height: 100%;
+  }
+
+  &.close[open] {
+    animation: fade-out $animation-duration forwards;
+
+    &::backdrop {
+      animation: fade-out $animation-duration forwards;
+    }
+  }
+
+  &[open] {
+    animation: fade-in $animation-duration forwards;
+
+    &::backdrop {
+      animation: fade-in $animation-duration forwards;
+    }
+  }
 }
 
-.backdrop {
-  position: absolute;
-  top: 0;
-  left: 0;
-  width: 100%;
-  height: 100%;
+dialog::backdrop {
   background-color: theme.$modal-backdrop-bg;
 }
 
-.container {
-  display: flex;
-  justify-content: center;
-  align-items: center;
-  height: 100%;
+@keyframes fade-in {
+  from {
+    opacity: 0;
+  }
+  to {
+    opacity: 1;
+  }
+}
+
+@keyframes fade-out {
+  from {
+    opacity: 1;
+  }
+  to {
+    opacity: 0;
+  }
 }