From d7c7cab6af1e04d502997bb2007b85630fe92089 Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 17:37:56 -0700 Subject: [PATCH 01/14] remove a few things --- src/components/Dialog/index.tsx | 48 ++++++++++++++--------------- src/components/Dialog/index.web.tsx | 27 +++++++--------- 2 files changed, 34 insertions(+), 41 deletions(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index 1249897b2b..7462ee83ce 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -12,7 +12,6 @@ import BottomSheet, { WINDOW_HEIGHT, } from '@discord/bottom-sheet/src' -import {logger} from '#/logger' import {useDialogStateControlContext} from '#/state/dialogs' import {isNative} from 'platform/detection' import {atoms as a, flatten, useTheme} from '#/alf' @@ -105,19 +104,34 @@ export function Outer({ [setOpenIndex, setDialogIsOpen, control.id], ) + // This is the function that we call when we want to dismiss the dialog. const close = React.useCallback(cb => { - if (cb && typeof cb === 'function') { - if (closeCallback.current) { - logger.error( - `Dialog close was passed multiple callbacks, you shouldn't do that`, - ) - } + if (typeof cb === 'function') { closeCallback.current = 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) + + try { + if (closeCallback.current) { + closeCallback.current() + } + } catch (e) { + // No need to handle this here + } finally { + onClose?.() + setDialogIsOpen(control.id, false) + } + }, [control.id, onClose, setDialogIsOpen]) + useImperativeHandle( control.ref, () => ({ @@ -127,22 +141,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 +168,7 @@ export function Outer({ backdropComponent={Backdrop} handleIndicatorStyle={{backgroundColor: t.palette.primary_500}} handleStyle={{display: 'none'}} - onClose={onCloseInner}> + onClose={onCloseAnimationComplete}> { - setIsOpen(true) setDialogIsOpen(control.id, true) + setIsOpen(true) }, [setIsOpen, setDialogIsOpen, control.id]) const close = React.useCallback( cb => { setDialogIsOpen(control.id, false) - setIsVisible(false) setIsOpen(false) - setIsVisible(true) try { if (cb && typeof cb === 'function') { @@ -113,17 +110,15 @@ export function Outer({ gtMobile ? a.p_lg : a.p_md, {overflowY: 'auto'}, ]}> - {isVisible && ( - - )} + - {isVisible ? children : null} + {children} From 7af74dcfc7d062b6e46ebdcac00475f8f4fb9764 Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 17:48:56 -0700 Subject: [PATCH 02/14] oops --- src/components/Dialog/index.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index 7462ee83ce..f44318b24c 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -128,7 +128,6 @@ export function Outer({ // No need to handle this here } finally { onClose?.() - setDialogIsOpen(control.id, false) } }, [control.id, onClose, setDialogIsOpen]) From deb4b27b7e2cc322921b5fa63908514cdad3c003 Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 18:19:59 -0700 Subject: [PATCH 03/14] stop swallowing the error --- src/components/Dialog/index.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index f44318b24c..ab9c55aa45 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -124,8 +124,6 @@ export function Outer({ if (closeCallback.current) { closeCallback.current() } - } catch (e) { - // No need to handle this here } finally { onClose?.() } From 121840c3e07ef7036ada6fee655a7b60a7287124 Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 18:31:46 -0700 Subject: [PATCH 04/14] queue callbacks --- src/components/Dialog/index.tsx | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index ab9c55aa45..31772417ba 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -82,7 +82,8 @@ export function Outer({ const sheetOptions = nativeOptions?.sheet || {} const hasSnapPoints = !!sheetOptions.snapPoints const insets = useSafeAreaInsets() - const closeCallback = React.useRef<() => void>() + const isClosing = React.useRef(false) + const closeCallbacks = React.useRef<(() => void)[]>([]) const {setDialogIsOpen} = useDialogStateControlContext() /* @@ -107,9 +108,14 @@ export function Outer({ // This is the function that we call when we want to dismiss the dialog. const close = React.useCallback(cb => { if (typeof cb === 'function') { - closeCallback.current = cb + closeCallbacks.current?.push(cb) + closeCallbacks.current?.push(cb) + } + + if (!isClosing.current) { + sheet.current?.close() + isClosing.current = true } - sheet.current?.close() }, []) // This is the actual thing we are doing once we "confirm" the dialog. We want the dialog's close animation to @@ -120,13 +126,14 @@ export function Outer({ setDialogIsOpen(control.id, false) setOpenIndex(-1) - try { - if (closeCallback.current) { - closeCallback.current() + if (closeCallbacks.current && closeCallbacks.current.length > 0) { + for (const cb of closeCallbacks.current) { + try { + cb() + } catch {} } - } finally { - onClose?.() } + onClose?.() }, [control.id, onClose, setDialogIsOpen]) useImperativeHandle( From c9831f9f19f33890f297c808eac06f5260e01b2d Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 18:32:29 -0700 Subject: [PATCH 05/14] oops --- src/components/Dialog/index.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index 31772417ba..8f4a5a0777 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -109,7 +109,6 @@ export function Outer({ const close = React.useCallback(cb => { if (typeof cb === 'function') { closeCallbacks.current?.push(cb) - closeCallbacks.current?.push(cb) } if (!isClosing.current) { From e98ebae47685b1c226de79c7b89a86fa750431c6 Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 18:36:41 -0700 Subject: [PATCH 06/14] log error if caught --- src/components/Dialog/index.tsx | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index 8f4a5a0777..0a9009bbe7 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -12,6 +12,7 @@ import BottomSheet, { WINDOW_HEIGHT, } from '@discord/bottom-sheet/src' +import {logger} from '#/logger' import {useDialogStateControlContext} from '#/state/dialogs' import {isNative} from 'platform/detection' import {atoms as a, flatten, useTheme} from '#/alf' @@ -129,7 +130,9 @@ export function Outer({ for (const cb of closeCallbacks.current) { try { cb() - } catch {} + } catch (e: any) { + logger.error('Error running close callback', e) + } } } onClose?.() From 84d5cebd98a964a0d35a8c2993db384fabf02f7b Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 18:57:32 -0700 Subject: [PATCH 07/14] no need to be nullable --- src/components/Dialog/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index 0a9009bbe7..48256f657e 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -109,7 +109,7 @@ export function Outer({ // This is the function that we call when we want to dismiss the dialog. const close = React.useCallback(cb => { if (typeof cb === 'function') { - closeCallbacks.current?.push(cb) + closeCallbacks.current.push(cb) } if (!isClosing.current) { From 8dfc0853b7b7d7bbbfb44918d5ce0de4b40aadfc Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 18:58:35 -0700 Subject: [PATCH 08/14] move isClosing=true up --- src/components/Dialog/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index 48256f657e..cc30bbd287 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -113,8 +113,8 @@ export function Outer({ } if (!isClosing.current) { - sheet.current?.close() isClosing.current = true + sheet.current?.close() } }, []) From bebdc8faf375dddaa02255ba29b9bc22cf023b2e Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 19:07:54 -0700 Subject: [PATCH 09/14] reset `isClosing` and `closeCallbacks` on close completion and open --- src/components/Dialog/index.tsx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index cc30bbd287..9368c2f5d9 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -102,6 +102,9 @@ export function Outer({ setDialogIsOpen(control.id, true) // can be set to any index of `snapPoints`, but `0` is the first i.e. "open" setOpenIndex(index || 0) + + isClosing.current = false + closeCallbacks.current = [] }, [setOpenIndex, setDialogIsOpen, control.id], ) @@ -136,6 +139,9 @@ export function Outer({ } } onClose?.() + + isClosing.current = false + closeCallbacks.current = [] }, [control.id, onClose, setDialogIsOpen]) useImperativeHandle( From 4042122eac01da35bbf7646ea8e1ed8df85b7458 Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 19:12:56 -0700 Subject: [PATCH 10/14] run queued callbacks on `open` if there are any pending --- src/components/Dialog/index.tsx | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index 9368c2f5d9..e4c3980eb5 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -97,16 +97,28 @@ 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( ({index} = {}) => { setDialogIsOpen(control.id, true) // can be set to any index of `snapPoints`, but `0` is the first i.e. "open" setOpenIndex(index || 0) + callQueuedCallbacks() isClosing.current = false - closeCallbacks.current = [] }, - [setOpenIndex, setDialogIsOpen, control.id], + [setDialogIsOpen, control.id, callQueuedCallbacks], ) // This is the function that we call when we want to dismiss the dialog. @@ -129,20 +141,10 @@ export function Outer({ setDialogIsOpen(control.id, false) setOpenIndex(-1) - if (closeCallbacks.current && closeCallbacks.current.length > 0) { - for (const cb of closeCallbacks.current) { - try { - cb() - } catch (e: any) { - logger.error('Error running close callback', e) - } - } - } + callQueuedCallbacks() onClose?.() - isClosing.current = false - closeCallbacks.current = [] - }, [control.id, onClose, setDialogIsOpen]) + }, [callQueuedCallbacks, control.id, onClose, setDialogIsOpen]) useImperativeHandle( control.ref, From 9f706f0af97e0ee4d948c6a7f2725a776f0f9215 Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 20:08:30 -0700 Subject: [PATCH 11/14] rm unnecessary ref and check --- src/components/Dialog/index.tsx | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index e4c3980eb5..50c10cd2fd 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -83,7 +83,6 @@ export function Outer({ const sheetOptions = nativeOptions?.sheet || {} const hasSnapPoints = !!sheetOptions.snapPoints const insets = useSafeAreaInsets() - const isClosing = React.useRef(false) const closeCallbacks = React.useRef<(() => void)[]>([]) const {setDialogIsOpen} = useDialogStateControlContext() @@ -116,7 +115,6 @@ export function Outer({ setOpenIndex(index || 0) callQueuedCallbacks() - isClosing.current = false }, [setDialogIsOpen, control.id, callQueuedCallbacks], ) @@ -126,11 +124,7 @@ export function Outer({ if (typeof cb === 'function') { closeCallbacks.current.push(cb) } - - if (!isClosing.current) { - isClosing.current = true - sheet.current?.close() - } + sheet.current?.close() }, []) // This is the actual thing we are doing once we "confirm" the dialog. We want the dialog's close animation to @@ -143,7 +137,6 @@ export function Outer({ callQueuedCallbacks() onClose?.() - isClosing.current = false }, [callQueuedCallbacks, control.id, onClose, setDialogIsOpen]) useImperativeHandle( From 082d63d1dee8860fdd0edd14ab433deb01d716c4 Mon Sep 17 00:00:00 2001 From: Hailey Date: Mon, 8 Apr 2024 20:17:26 -0700 Subject: [PATCH 12/14] ensure order of calls is always correct --- src/components/Dialog/index.web.tsx | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/components/Dialog/index.web.tsx b/src/components/Dialog/index.web.tsx index 9c715e5043..1892d944ed 100644 --- a/src/components/Dialog/index.web.tsx +++ b/src/components/Dialog/index.web.tsx @@ -47,7 +47,11 @@ export function Outer({ 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`, { From ef11e61edceb024a45d8cfdec9b0662c64c3a7d7 Mon Sep 17 00:00:00 2001 From: Hailey Date: Tue, 9 Apr 2024 10:01:53 -0700 Subject: [PATCH 13/14] call `snapToIndex()` on open --- src/components/Dialog/index.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index 50c10cd2fd..55798db7f5 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -110,11 +110,13 @@ export function Outer({ const open = React.useCallback( ({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) - - callQueuedCallbacks() + sheet.current?.snapToIndex(index || 0) }, [setDialogIsOpen, control.id, callQueuedCallbacks], ) From 7982c8fe63a71e527f95857c11d59f4d586a20de Mon Sep 17 00:00:00 2001 From: Hailey Date: Tue, 9 Apr 2024 10:09:17 -0700 Subject: [PATCH 14/14] add tester to storybook --- src/view/screens/Storybook/Dialogs.tsx | 137 ++++++++++++++++++++++++- 1 file changed, 136 insertions(+), 1 deletion(-) diff --git a/src/view/screens/Storybook/Dialogs.tsx b/src/view/screens/Storybook/Dialogs.tsx index 4722784cae..f68f9f4ddf 100644 --- 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() { Open prompt + + This is a prompt @@ -122,6 +132,131 @@ export function Dialogs() { + + + + + + + + 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. + + + + + + + + + + + + + + + + ) }