Simplify closeHandler in Dialog

src/components/feedback/Dialog.tsx

@@ -43,7 +43,7 @@ type ComponentProps = {
   restoreFocus?: boolean;
 
   /**
-   * Allows to provide a transition animation to the Dialog contents
+   * Optional TransitionComponent for open (mount) and close (unmount) transitions
    */
   transitionComponent?: TransitionComponent;
 };
@@ -74,7 +74,7 @@ const DialogNext = function Dialog({
   // Forwarded to Panel
   buttons,
   icon,
-  onClose,
+  onClose = noop,
   paddingSize = 'md',
   scrollable = true,
   title,
@@ -87,13 +87,18 @@ const DialogNext = function Dialog({
   );
   const [transitionComponentVisible, setTransitionComponentVisible] =
     useState(false);
-  // If a TransitionComponent was provided, closing the Dialog should just set
-  // it to not visible. The TransitionComponent will take care of actually
-  // closing the dialog once the "out" transition has finished
-  const closeHandler = TransitionComponent
-    ? () => setTransitionComponentVisible(false)
-    : onClose ?? noop;
-  const setInitialFocus = useCallback(() => {
+
+  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;
     }
@@ -117,12 +122,12 @@ const DialogNext = function Dialog({
       modalRef.current?.focus();
     }
   }, [initialFocus, modalRef]);
+
   const onTransitionEnd = (direction: 'in' | 'out') => {
     if (direction === 'in') {
-      // We can't check the initial focus until transition "in" has finished
-      setInitialFocus();
+      initializeDialog();
     } else {
-      onClose?.();
+      onClose();
     }
   };
 
@@ -145,10 +150,9 @@ const DialogNext = function Dialog({
   );
 
   useEffect(() => {
-    // Trigger initial "in" transition animation
     setTransitionComponentVisible(true);
     if (!TransitionComponent) {
-      setInitialFocus();
+      initializeDialog();
     }
 
     // We only want to run this effect once when the dialog is mounted.

src/components/feedback/test/Dialog-test.js

@@ -18,8 +18,8 @@ const createComponent = (Component, props = {}) => {
  * @type {import('../../../next.js').TransitionComponent}
  */
 const ComponentWithTransition = ({ children, visible, onTransitionEnd }) => {
-  // Fake a 200ms transition time
-  setTimeout(() => onTransitionEnd?.(visible ? 'in' : 'out'), 200);
+  // Fake a 50ms transition time
+  setTimeout(() => onTransitionEnd?.(visible ? 'in' : 'out'), 50);
   return <div>{children}</div>;
 };
 
@@ -137,7 +137,7 @@ describe('Dialog', () => {
           wrapper.find('[role="dialog"]').getDOMNode()
         );
         // Once the transition has ended, the Dialog should be focused
-        await delay(300); // Transition finishes after 200ms
+        await delay(60); // Transition finishes after 6ms
         assert.equal(
           document.activeElement,
           wrapper.find('[role="dialog"]').getDOMNode()
@@ -278,7 +278,7 @@ describe('Dialog', () => {
         // The onClose callback is not immediately invoked
         assert.notCalled(onClose);
         // Once the transition has ended, the callback should have been called
-        await delay(300); // Transition finishes after 200ms
+        await delay(60); // Transition finishes after 50ms
         assert.called(onClose);
       });
     });

src/pattern-library/components/patterns/FadeComponent.tsx

@@ -3,6 +3,10 @@ import { useCallback } from 'preact/hooks';
 
 import type { TransitionComponent } from '../../../types';
 
+/**
+ * This component is used just to demonstrate the `TransitionComponent`
+ * functionality on components supporting it, like `Dialog`.
+ */
 const FadeComponent: TransitionComponent = ({
   children,
   visible,
@@ -19,7 +23,7 @@ const FadeComponent: TransitionComponent = ({
         'opacity-100': visible,
         'opacity-0': !visible,
       })}
-      // @ts-expect-error react uses "ontransitionend" rather than "onTransitionEnd".
+      // @ts-expect-error preact uses "ontransitionend" rather than "onTransitionEnd".
       // eslint-disable-next-line react/no-unknown-property
       ontransitionend={handleTransitionEnd}
     >

src/types.ts

@@ -27,8 +27,8 @@ export type BaseProps = PresentationalProps & { unstyled?: boolean };
 export type IconComponent = FunctionComponent<JSX.SVGAttributes<SVGSVGElement>>;
 
 /**
- * A component type used by other components that can optionally support a
- * transition animation
+ * A component that describes an `in` and an `out` transition, typically to
+ * animate the mounting and unmounting of a child component.
  */
 export type TransitionComponent = JSX.ElementType<{
   visible: boolean;