Try to solve closing Dialogs with transition using useImperativeHandle

[acelaya]

As described in hypothesis/client#5520, when a Dialog is closed from the outside using the raw callback that was passed to onClose, the out transition is not respected, as that's an internal implementation detail.

An attempt to solve this was done in #1131, but the result was too complex and convoluted, that we decided to not go that way.

We want to try and use useImperativeHandle in order to expose the closeHandler that wraps onClose, but taking the transition into consideration.

That way, consumers would be able to properly close the Dialog with a transition.

[acelaya]

Out of curiosity, I quickly checked what would be the implication to get this done.

The changes in this library are pretty straightforward. We would need to decide of the "enhanced" ref is the same one the Dialog can already get, or if we should use a separated one.

The way TypeScript complains, it feels the right path is the second, but on the other hand, being able to use the same ref would simplify it even further.

The problem comes when we want to make use of this in client. I think we made the assumption that the callback would need to be invoked in place, from the component rendering the Dialog.

In hypothesis/client#5520, the panel is unmounted via a change in the shared store triggered from a component somewhere else.

I guess we would need to sync states somehow, but this needs a bit more thinking.

[acelaya]

I finally put together a working POC.

Solving hypothesis/client#5520

As mentioned in previous comment, the changes in this library should be straightforward. Let's assume the method exposed in this new ref is called closeWithTransition.

close-with-transition.webm

The changes in SidebarPanel would look like this:

diff --git a/src/sidebar/components/SidebarPanel.tsx b/src/sidebar/components/SidebarPanel.tsx
index 213373f77..963142f37 100644
--- a/src/sidebar/components/SidebarPanel.tsx
+++ b/src/sidebar/components/SidebarPanel.tsx
@@ -1,7 +1,7 @@
 import { Dialog, Slider } from '@hypothesis/frontend-shared';
 import type { IconComponent } from '@hypothesis/frontend-shared/lib/types';
 import type { ComponentChildren } from 'preact';
-import { useCallback, useEffect, useRef } from 'preact/hooks';
+import { useCallback, useEffect, useRef, useState } from 'preact/hooks';
 import scrollIntoView from 'scroll-into-view';
 
 import type { PanelName } from '../../types/sidebar';
@@ -38,8 +38,10 @@ export default function SidebarPanel({
 }: SidebarPanelProps) {
   const store = useSidebarStore();
   const panelIsActive = store.isSidebarPanelOpen(panelName);
+  const [panelIsVisible, setPanelIsVisible] = useState(panelIsActive);
 
   const panelElement = useRef<HTMLDivElement | null>(null);
+  const controlsRef = useRef();
   const panelWasActive = useRef(panelIsActive);
 
   // Scroll the panel into view if it has just been opened
@@ -53,13 +55,25 @@ export default function SidebarPanel({
     }
   }, [panelIsActive, onActiveChanged]);
 
+  useEffect(() => {
+    if (panelIsActive) {
+      // Once the panel is active, we immediately make it visible
+      setPanelIsVisible(true);
+    } else {
+      // When the panel is deactivated we invoke close with transition, so that
+      // hiding it is delayed until transition has finished
+      controlsRef.current?.closeWithTransition();
+    }
+  }, [panelIsActive]);
+
   const closePanel = useCallback(() => {
+    setPanelIsVisible(false);
     store.toggleSidebarPanel(panelName, false);
   }, [store, panelName]);
 
   return (
     <>
-      {panelIsActive && (
+      {panelIsVisible && (
         <Dialog
           restoreFocus
           ref={panelElement}
@@ -69,6 +83,7 @@ export default function SidebarPanel({
           onClose={closePanel}
           transitionComponent={Slider}
           variant={variant}
+          controlsRef={controlsRef}
         >
           {children}
         </Dialog>

The main "ugliness" introduced here is that deactivating it was happening due to a shared store change.

In order to address that we need to introduce a new piece of local state, used to unmount the Dialog, which is kept in sync asyncronously.

When the shared store flag panelIsActive is set to true, we set the local state to true, but when it is set to false, we call controlsRef.current?.closeWithTransition() which is exposed by the Dialog via useImperativeHandle, delaying the actual call to closePanel until the transition finished.

Simpler use case

For components not using shared state, but local one, using closeWithTransition would be even simpler.

const [isClosed, setIsClosed] = useState(true);
+const controlsRef = useRef();

return (
  <>
-    <button onClick={() => setIsClosed(true)}>Close dialog</button>
+    <button onClick={() => controlsRef.current?.closeWithTransition()}>Close dialog</button>

    {!isClosed && (
      <Dialog 
        onClose={() => setIsClosed(true)}
        transitionComponent={Slider}
+       controlsRef={controlsRef}
      />
    )}
  </>
)