From 9f5ca949856f66e11608e07202fdbdd3406fefc2 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandro.celaya@hypothes.is>
Date: Tue, 4 Jul 2023 11:13:32 +0200
Subject: [PATCH] Extract transition component handling to
 useTransitionComponent hook

---
 src/components/feedback/Dialog.tsx     | 130 +++++++------------------
 src/hooks/use-transition-component.tsx | 104 ++++++++++++++++++++
 src/index.ts                           |   1 +
 3 files changed, 141 insertions(+), 94 deletions(-)
 create mode 100644 src/hooks/use-transition-component.tsx

diff --git a/src/components/feedback/Dialog.tsx b/src/components/feedback/Dialog.tsx
index 688ffa472..0e09b718a 100644
--- a/src/components/feedback/Dialog.tsx
+++ b/src/components/feedback/Dialog.tsx
@@ -1,19 +1,12 @@
 import classnames from 'classnames';
 import type { RefObject } from 'preact';
-import { Fragment } from 'preact';
-import {
-  useCallback,
-  useEffect,
-  useLayoutEffect,
-  useMemo,
-  useRef,
-  useState,
-} from 'preact/hooks';
+import { useCallback, useLayoutEffect, useRef } from 'preact/hooks';
 
 import { useClickAway } from '../../hooks/use-click-away';
 import { useFocusAway } from '../../hooks/use-focus-away';
 import { useKeyPress } from '../../hooks/use-key-press';
 import { useSyncedRef } from '../../hooks/use-synced-ref';
+import { useTransitionComponent } from '../../hooks/use-transition-component';
 import { useUniqueId } from '../../hooks/use-unique-id';
 import type { PresentationalProps, TransitionComponent } from '../../types';
 import { downcastRef } from '../../util/typing';
@@ -72,7 +65,7 @@ const Dialog = function Dialog({
   children,
   initialFocus = 'auto',
   restoreFocus = false,
-  transitionComponent: TransitionComponent,
+  transitionComponent,
   closed = false,
 
   classes,
@@ -92,23 +85,6 @@ const Dialog = function Dialog({
   const restoreFocusEl = useRef<HTMLElement | null>(null);
   const isClosableDialog = !!onClose;
 
-  // TODO To properly handle closing/opening with a TransitionComponent, these
-  //  two pieces of state need to be synced with the `closed` prop.
-  //  That makes the logic hard to follow. It would be good to revisit eventually
-  const [isClosed, setIsClosed] = useState(closed);
-  const [transitionComponentVisible, setTransitionComponentVisible] =
-    useState(false);
-
-  const closeHandler = useCallback(() => {
-    if (TransitionComponent) {
-      // When a TransitionComponent is provided, the actual "onClose" will be
-      // called by that component, once the "out" transition has finished
-      setTransitionComponentVisible(false);
-    } else {
-      onClose?.();
-    }
-  }, [onClose, TransitionComponent]);
-
   const initializeDialog = useCallback(() => {
     if (initialFocus === 'manual') {
       return;
@@ -134,21 +110,21 @@ const Dialog = function Dialog({
     }
   }, [initialFocus, modalRef]);
 
+  const { closeHandler, isClosed, wrapWithTransition } = useTransitionComponent(
+    {
+      closed,
+      transitionComponent,
+      onClose,
+      onOpen: initializeDialog,
+    }
+  );
+
   const doRestoreFocus = useCallback(() => {
     if (restoreFocus) {
       restoreFocusEl.current?.focus();
     }
   }, [restoreFocus]);
 
-  const onTransitionEnd = (direction: 'in' | 'out') => {
-    if (direction === 'in') {
-      initializeDialog();
-    } else {
-      setIsClosed(true);
-      onClose?.();
-    }
-  };
-
   useClickAway(modalRef, closeHandler, {
     enabled: closeOnClickAway,
   });
@@ -162,35 +138,6 @@ const Dialog = function Dialog({
   });
 
   const dialogDescriptionId = useUniqueId('dialog-description');
-  const Wrapper = useMemo(
-    () => TransitionComponent ?? Fragment,
-    [TransitionComponent]
-  );
-
-  useEffect(() => {
-    if (closed) {
-      if (TransitionComponent) {
-        setTransitionComponentVisible(false);
-      } else {
-        setIsClosed(true);
-      }
-    } else {
-      setIsClosed(false);
-    }
-
-    if (!closed && !TransitionComponent) {
-      initializeDialog();
-    }
-
-    // We only want to run this effect when opened or closed.
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [closed]);
-
-  useEffect(() => {
-    if (!isClosed) {
-      setTransitionComponentVisible(true);
-    }
-  }, [isClosed]);
 
   useLayoutEffect(() => {
     if (!isClosed) {
@@ -225,37 +172,32 @@ const Dialog = function Dialog({
     return null;
   }
 
-  return (
-    <Wrapper
-      direction={transitionComponentVisible ? 'in' : 'out'}
-      onTransitionEnd={onTransitionEnd}
+  return wrapWithTransition(
+    <div
+      data-component="Dialog"
+      tabIndex={-1}
+      // NB: Role can be overridden with an HTML attribute; this is purposeful
+      role="dialog"
+      {...htmlAttributes}
+      className={classnames(
+        // Column-flex layout to constrain content to max-height
+        'flex flex-col',
+        classes
+      )}
+      ref={downcastRef(modalRef)}
     >
-      <div
-        data-component="Dialog"
-        tabIndex={-1}
-        // NB: Role can be overridden with an HTML attribute; this is purposeful
-        role="dialog"
-        {...htmlAttributes}
-        className={classnames(
-          // Column-flex layout to constrain content to max-height
-          'flex flex-col',
-          classes
-        )}
-        ref={downcastRef(modalRef)}
+      <Panel
+        buttons={buttons}
+        fullWidthHeader={true}
+        icon={icon}
+        onClose={isClosableDialog ? closeHandler : undefined}
+        paddingSize={paddingSize}
+        title={title}
+        scrollable={scrollable}
       >
-        <Panel
-          buttons={buttons}
-          fullWidthHeader={true}
-          icon={icon}
-          onClose={isClosableDialog ? closeHandler : undefined}
-          paddingSize={paddingSize}
-          title={title}
-          scrollable={scrollable}
-        >
-          {children}
-        </Panel>
-      </div>
-    </Wrapper>
+        {children}
+      </Panel>
+    </div>
   );
 };
 
diff --git a/src/hooks/use-transition-component.tsx b/src/hooks/use-transition-component.tsx
new file mode 100644
index 000000000..73fe709e5
--- /dev/null
+++ b/src/hooks/use-transition-component.tsx
@@ -0,0 +1,104 @@
+import type { ComponentChildren } from 'preact';
+import type { JSX } from 'preact';
+import { useCallback, useEffect, useState } from 'preact/hooks';
+
+import type { TransitionComponent } from '../types';
+
+type TransitionComponentOptions = {
+  closed: boolean;
+  transitionComponent?: TransitionComponent;
+  onClose?: () => void;
+  onOpen?: () => void;
+};
+
+type TransitionComponentResult = {
+  closeHandler: () => void;
+  isClosed: boolean;
+  wrapWithTransition: (children: ComponentChildren) => JSX.Element;
+};
+
+export function useTransitionComponent({
+  closed,
+  transitionComponent,
+  onClose,
+  onOpen,
+}: TransitionComponentOptions): TransitionComponentResult {
+  // TODO To properly handle closing/opening with a TransitionComponent, these
+  //  two pieces of state need to be synced with the `closed` argument.
+  //  That makes the logic hard to follow. It would be good to revisit eventually
+  const [isClosed, setIsClosed] = useState(closed);
+  const [transitionComponentVisible, setTransitionComponentVisible] =
+    useState(false);
+
+  const closeHandler = useCallback(() => {
+    if (transitionComponent) {
+      // When a TransitionComponent is provided, the actual "onClose" will be
+      // called by that component, once the "out" transition has finished
+      setTransitionComponentVisible(false);
+    } else {
+      onClose?.();
+    }
+  }, [onClose, transitionComponent]);
+
+  const onTransitionEnd = useCallback(
+    (direction: 'in' | 'out') => {
+      if (direction === 'in') {
+        onOpen?.();
+      } else {
+        setIsClosed(true);
+        onClose?.();
+      }
+    },
+    [onOpen, onClose]
+  );
+
+  useEffect(() => {
+    if (closed) {
+      if (transitionComponent) {
+        setTransitionComponentVisible(false);
+      } else {
+        setIsClosed(true);
+      }
+    } else {
+      setIsClosed(false);
+    }
+
+    if (!closed && !transitionComponent) {
+      onOpen?.();
+    }
+
+    // We only want to run this effect when opened or closed.
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [closed]);
+
+  useEffect(() => {
+    if (!isClosed) {
+      setTransitionComponentVisible(true);
+    }
+  }, [isClosed]);
+
+  const wrapWithTransition = useCallback(
+    (children: ComponentChildren) => {
+      if (!transitionComponent) {
+        return <>{children}</>;
+      }
+
+      const TransitionComp = transitionComponent;
+      return (
+        <TransitionComp
+          direction={transitionComponentVisible ? 'in' : 'out'}
+          onTransitionEnd={onTransitionEnd}
+        >
+          {children}
+        </TransitionComp>
+      );
+    },
+    [transitionComponent, transitionComponentVisible, onTransitionEnd]
+  );
+
+  return {
+    closeHandler,
+    isClosed,
+    wrapWithTransition,
+  };
+}
diff --git a/src/index.ts b/src/index.ts
index 0261e9e5b..f84892df7 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -4,6 +4,7 @@ export { useClickAway } from './hooks/use-click-away';
 export { useFocusAway } from './hooks/use-focus-away';
 export { useKeyPress } from './hooks/use-key-press';
 export { useSyncedRef } from './hooks/use-synced-ref';
+export { useTransitionComponent } from './hooks/use-transition-component';
 
 // Components
 export * from './components/icons';