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
bluesky-social · Apr 9, 2024 · 162059a · 162059a
1 parent 027d6e5
commit 162059a
Show file tree

Hide file tree

Showing 3 changed files with 186 additions and 45 deletions.
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,28 +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') {
-      if (closeCallback.current) {
-        logger.error(
-          `Dialog close was passed multiple callbacks, you shouldn't do that`,
-        )
-      }
-      closeCallback.current = cb
+    if (typeof cb === 'function') {
+      closeCallbacks.current.push(cb)
     }
-    // initiates a close animation, the actual "close" happens in `onCloseInner`
     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,
     () => ({
@@ -127,22 +150,6 @@ export function Outer({
     [open, close],
   )
 
-  const onCloseInner = React.useCallback(() => {
-    setDialogIsOpen(control.id, false)
-    setOpenIndex(-1)
-    try {
-      logger.debug(`Dialog closeCallback`, {controlId: control.id})
-      closeCallback.current?.()
-    } catch (e: any) {
-      logger.error(`Dialog closeCallback failed`, {
-        message: e.message,
-      })
-    } finally {
-      closeCallback.current = undefined
-    }
-    onClose?.()
-  }, [control.id, onClose, setDialogIsOpen])
-
   const context = React.useMemo(() => ({close}), [close])
 
   return (
@@ -170,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,24 +33,25 @@ 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 close = React.useCallback<DialogControlProps['close']>(
     cb => {
       setDialogIsOpen(control.id, false)
-      setIsVisible(false)
       setIsOpen(false)
-      setIsVisible(true)
 
       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`, {
@@ -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/view/screens/Storybook/Dialogs.tsx b/src/view/screens/Storybook/Dialogs.tsx
@@ -6,12 +6,13 @@ import {atoms as a} from '#/alf'
 import {Button, ButtonText} from '#/components/Button'
 import * as Dialog from '#/components/Dialog'
 import * as Prompt from '#/components/Prompt'
-import {H3, P} from '#/components/Typography'
+import {H3, P, Text} from '#/components/Typography'
 
 export function Dialogs() {
   const scrollable = Dialog.useDialogControl()
   const basic = Dialog.useDialogControl()
   const prompt = Prompt.usePromptControl()
+  const testDialog = Dialog.useDialogControl()
   const {closeAllDialogs} = useDialogStateControlContext()
 
   return (
@@ -60,6 +61,15 @@ export function Dialogs() {
         <ButtonText>Open prompt</ButtonText>
       </Button>
 
+      <Button
+        variant="solid"
+        color="primary"
+        size="small"
+        onPress={testDialog.open}
+        label="one">
+        <ButtonText>Open Tester</ButtonText>
+      </Button>
+
       <Prompt.Outer control={prompt}>
         <Prompt.TitleText>This is a prompt</Prompt.TitleText>
         <Prompt.DescriptionText>
@@ -122,6 +132,131 @@ export function Dialogs() {
           </View>
         </Dialog.ScrollableInner>
       </Dialog.Outer>
+
+      <Dialog.Outer control={testDialog}>
+        <Dialog.Handle />
+
+        <Dialog.ScrollableInner
+          accessibilityDescribedBy="dialog-description"
+          accessibilityLabelledBy="dialog-title">
+          <View style={[a.relative, a.gap_md, a.w_full]}>
+            <Text>
+              Watch the console logs to test each of these dialog edge cases.
+              Functionality should be consistent across both native and web. If
+              not then *sad face* something is wrong.
+            </Text>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                testDialog.close(() => {
+                  console.log('close callback')
+                })
+              }}
+              label="Close It">
+              <ButtonText>Normal Use (Should just log)</ButtonText>
+            </Button>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                testDialog.close(() => {
+                  console.log('close callback')
+                })
+
+                setTimeout(() => {
+                  testDialog.open()
+                }, 100)
+              }}
+              label="Close It">
+              <ButtonText>
+                Calls `.open()` in 100ms (Should log when the animation switches
+                to open)
+              </ButtonText>
+            </Button>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                setTimeout(() => {
+                  testDialog.open()
+                }, 2e3)
+
+                testDialog.close(() => {
+                  console.log('close callback')
+                })
+              }}
+              label="Close It">
+              <ButtonText>
+                Calls `.open()` in 2000ms (Should log after close animation and
+                not log on open)
+              </ButtonText>
+            </Button>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                testDialog.close(() => {
+                  console.log('close callback')
+                })
+                setTimeout(() => {
+                  testDialog.close(() => {
+                    console.log('close callback after 100ms')
+                  })
+                }, 100)
+              }}
+              label="Close It">
+              <ButtonText>
+                Calls `.close()` then again in 100ms (should log twice)
+              </ButtonText>
+            </Button>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                testDialog.close(() => {
+                  console.log('close callback')
+                })
+                testDialog.close(() => {
+                  console.log('close callback 2')
+                })
+              }}
+              label="Close It">
+              <ButtonText>
+                Call `close()` twice immediately (should just log twice)
+              </ButtonText>
+            </Button>
+
+            <Button
+              variant="outline"
+              color="primary"
+              size="small"
+              onPress={() => {
+                console.log('Step 1')
+                testDialog.close(() => {
+                  console.log('Step 3')
+                })
+                console.log('Step 2')
+              }}
+              label="Close It">
+              <ButtonText>
+                Log before `close()`, after `close()` and in the `close()`
+                callback. Should be an order of 1 2 3
+              </ButtonText>
+            </Button>
+          </View>
+        </Dialog.ScrollableInner>
+      </Dialog.Outer>
     </View>
   )
 }