Small logic cleanups (#3449)

* Small logic cleanups * Small logic cleanups (#3451) * remove a few things * oops * stop swallowing the error * queue callbacks * oops * log error if caught * no need to be nullable * move isClosing=true up * reset `isClosing` and `closeCallbacks` on close completion and open * run queued callbacks on `open` if there are any pending * rm unnecessary ref and check * ensure order of calls is always correct * call `snapToIndex()` on open * add tester to storybook --------- Co-authored-by: Hailey <[email protected]>
bluesky-social · Apr 9, 2024 · c96bc92 · c96bc92
1 parent a49a5a3
commit c96bc92
Show file tree

Hide file tree

Showing 6 changed files with 213 additions and 54 deletions.
diff --git a/src/components/Dialog/context.ts b/src/components/Dialog/context.ts
@@ -39,8 +39,7 @@ export function useDialogControl(): DialogOuterProps['control'] {
         control.current.open()
       },
       close: cb => {
-        control.current.close()
-        cb?.()
+        control.current.close(cb)
       },
     }),
     [id, control],

diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx
@@ -83,7 +83,7 @@ export function Outer({
   const sheetOptions = nativeOptions?.sheet || {}
   const hasSnapPoints = !!sheetOptions.snapPoints
   const insets = useSafeAreaInsets()
-  const closeCallback = React.useRef<() => void>()
+  const closeCallbacks = React.useRef<(() => void)[]>([])
   const {setDialogIsOpen} = useDialogStateControlContext()
 
   /*
@@ -96,22 +96,51 @@ export function Outer({
    */
   const isOpen = openIndex > -1
 
+  const callQueuedCallbacks = React.useCallback(() => {
+    for (const cb of closeCallbacks.current) {
+      try {
+        cb()
+      } catch (e: any) {
+        logger.error('Error running close callback', e)
+      }
+    }
+
+    closeCallbacks.current = []
+  }, [])
+
   const open = React.useCallback<DialogControlProps['open']>(
     ({index} = {}) => {
+      // Run any leftover callbacks that might have been queued up before calling `.open()`
+      callQueuedCallbacks()
+
       setDialogIsOpen(control.id, true)
       // can be set to any index of `snapPoints`, but `0` is the first i.e. "open"
       setOpenIndex(index || 0)
+      sheet.current?.snapToIndex(index || 0)
     },
-    [setOpenIndex, setDialogIsOpen, control.id],
+    [setDialogIsOpen, control.id, callQueuedCallbacks],
   )
 
+  // This is the function that we call when we want to dismiss the dialog.
   const close = React.useCallback<DialogControlProps['close']>(cb => {
-    if (cb && typeof cb === 'function') {
-      closeCallback.current = cb
+    if (typeof cb === 'function') {
+      closeCallbacks.current.push(cb)
     }
     sheet.current?.close()
   }, [])
 
+  // This is the actual thing we are doing once we "confirm" the dialog. We want the dialog's close animation to
+  // happen before we run this. It is passed to the `BottomSheet` component.
+  const onCloseAnimationComplete = React.useCallback(() => {
+    // This removes the dialog from our list of stored dialogs. Not super necessary on iOS, but on Android this
+    // tells us that we need to toggle the accessibility overlay setting
+    setDialogIsOpen(control.id, false)
+    setOpenIndex(-1)
+
+    callQueuedCallbacks()
+    onClose?.()
+  }, [callQueuedCallbacks, control.id, onClose, setDialogIsOpen])
+
   useImperativeHandle(
     control.ref,
     () => ({
@@ -121,21 +150,6 @@ export function Outer({
     [open, close],
   )
 
-  const onCloseInner = React.useCallback(() => {
-    try {
-      closeCallback.current?.()
-    } catch (e: any) {
-      logger.error(`Dialog closeCallback failed`, {
-        message: e.message,
-      })
-    } finally {
-      closeCallback.current = undefined
-    }
-    setDialogIsOpen(control.id, false)
-    onClose?.()
-    setOpenIndex(-1)
-  }, [control.id, onClose, setDialogIsOpen])
-
   const context = React.useMemo(() => ({close}), [close])
 
   return (
@@ -163,7 +177,7 @@ export function Outer({
             backdropComponent={Backdrop}
             handleIndicatorStyle={{backgroundColor: t.palette.primary_500}}
             handleStyle={{display: 'none'}}
-            onClose={onCloseInner}>
+            onClose={onCloseAnimationComplete}>
             <Context.Provider value={context}>
               <View
                 style={[

diff --git a/src/components/Dialog/index.web.tsx b/src/components/Dialog/index.web.tsx
@@ -33,40 +33,41 @@ export function Outer({
   const t = useTheme()
   const {gtMobile} = useBreakpoints()
   const [isOpen, setIsOpen] = React.useState(false)
-  const [isVisible, setIsVisible] = React.useState(true)
   const {setDialogIsOpen} = useDialogStateControlContext()
 
   const open = React.useCallback(() => {
-    setIsOpen(true)
     setDialogIsOpen(control.id, true)
+    setIsOpen(true)
   }, [setIsOpen, setDialogIsOpen, control.id])
 
-  const onCloseInner = React.useCallback(async () => {
-    setIsVisible(false)
-    await new Promise(resolve => setTimeout(resolve, 150))
-    setIsOpen(false)
-    setIsVisible(true)
-    setDialogIsOpen(control.id, false)
-    onClose?.()
-  }, [control.id, onClose, setDialogIsOpen])
-
   const close = React.useCallback<DialogControlProps['close']>(
     cb => {
+      setDialogIsOpen(control.id, false)
+      setIsOpen(false)
+
       try {
         if (cb && typeof cb === 'function') {
-          cb()
+          // This timeout ensures that the callback runs at the same time as it would on native. I.e.
+          // console.log('Step 1') -> close(() => console.log('Step 3')) -> console.log('Step 2')
+          // This should always output 'Step 1', 'Step 2', 'Step 3', but without the timeout it would output
+          // 'Step 1', 'Step 3', 'Step 2'.
+          setTimeout(cb)
         }
       } catch (e: any) {
         logger.error(`Dialog closeCallback failed`, {
           message: e.message,
         })
-      } finally {
-        onCloseInner()
       }
+
+      onClose?.()
     },
-    [onCloseInner],
+    [control.id, onClose, setDialogIsOpen],
   )
 
+  const handleBackgroundPress = React.useCallback(async () => {
+    close()
+  }, [close])
+
   useImperativeHandle(
     control.ref,
     () => ({
@@ -103,7 +104,7 @@ export function Outer({
             <TouchableWithoutFeedback
               accessibilityHint={undefined}
               accessibilityLabel={_(msg`Close active dialog`)}
-              onPress={onCloseInner}>
+              onPress={handleBackgroundPress}>
               <View
                 style={[
                   web(a.fixed),
@@ -113,17 +114,15 @@ export function Outer({
                   gtMobile ? a.p_lg : a.p_md,
                   {overflowY: 'auto'},
                 ]}>
-                {isVisible && (
-                  <Animated.View
-                    entering={FadeIn.duration(150)}
-                    // exiting={FadeOut.duration(150)}
-                    style={[
-                      web(a.fixed),
-                      a.inset_0,
-                      {opacity: 0.8, backgroundColor: t.palette.black},
-                    ]}
-                  />
-                )}
+                <Animated.View
+                  entering={FadeIn.duration(150)}
+                  // exiting={FadeOut.duration(150)}
+                  style={[
+                    web(a.fixed),
+                    a.inset_0,
+                    {opacity: 0.8, backgroundColor: t.palette.black},
+                  ]}
+                />
 
                 <View
                   style={[
@@ -135,7 +134,7 @@ export function Outer({
                       minHeight: web('calc(90vh - 36px)') || undefined,
                     },
                   ]}>
-                  {isVisible ? children : null}
+                  {children}
                 </View>
               </View>
             </TouchableWithoutFeedback>

diff --git a/src/components/Prompt.tsx b/src/components/Prompt.tsx
@@ -123,6 +123,13 @@ export function Action({
   cta,
   testID,
 }: {
+  /**
+   * Callback to run when the action is pressed. The method is called _after_
+   * the dialog closes.
+   *
+   * Note: The dialog will close automatically when the action is pressed, you
+   * should NOT close the dialog as a side effect of this method.
+   */
   onPress: () => void
   color?: ButtonColor
   /**
@@ -165,6 +172,13 @@ export function Basic({
   description: string
   cancelButtonCta?: string
   confirmButtonCta?: string
+  /**
+   * Callback to run when the Confirm button is pressed. The method is called
+   * _after_ the dialog closes.
+   *
+   * Note: The dialog will close automatically when the action is pressed, you
+   * should NOT close the dialog as a side effect of this method.
+   */
   onConfirm: () => void
   confirmButtonColor?: ButtonColor
 }>) {

diff --git a/src/view/com/composer/Composer.tsx b/src/view/com/composer/Composer.tsx
@@ -507,9 +507,7 @@ export const ComposePost = observer(function ComposePost({
         control={discardPromptControl}
         title={_(msg`Discard draft?`)}
         description={_(msg`Are you sure you'd like to discard this draft?`)}
-        onConfirm={() => {
-          discardPromptControl.close(onClose)
-        }}
+        onConfirm={onClose}
         confirmButtonCta={_(msg`Discard`)}
         confirmButtonColor="negative"
       />