From 027d6e542147833ae5a4a4b5447af6ef6d56c1a5 Mon Sep 17 00:00:00 2001 From: Eric Bailey Date: Mon, 8 Apr 2024 16:56:21 -0500 Subject: [PATCH 1/2] Small logic cleanups --- src/components/Dialog/context.ts | 3 +-- src/components/Dialog/index.tsx | 11 +++++++++-- src/components/Dialog/index.web.tsx | 26 +++++++++++++------------- src/components/Prompt.tsx | 14 ++++++++++++++ src/view/com/composer/Composer.tsx | 4 +--- 5 files changed, 38 insertions(+), 20 deletions(-) diff --git a/src/components/Dialog/context.ts b/src/components/Dialog/context.ts index df8bbb0810..859f8edd77 100644 --- 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 index 07e101f85c..1249897b2b 100644 --- a/src/components/Dialog/index.tsx +++ b/src/components/Dialog/index.tsx @@ -107,8 +107,14 @@ export function Outer({ 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`, + ) + } closeCallback.current = cb } + // initiates a close animation, the actual "close" happens in `onCloseInner` sheet.current?.close() }, []) @@ -122,7 +128,10 @@ export function Outer({ ) 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`, { @@ -131,9 +140,7 @@ export function Outer({ } finally { closeCallback.current = undefined } - setDialogIsOpen(control.id, false) onClose?.() - setOpenIndex(-1) }, [control.id, onClose, setDialogIsOpen]) const context = React.useMemo(() => ({close}), [close]) diff --git a/src/components/Dialog/index.web.tsx b/src/components/Dialog/index.web.tsx index 8383979b3c..8187c16f1a 100644 --- a/src/components/Dialog/index.web.tsx +++ b/src/components/Dialog/index.web.tsx @@ -41,17 +41,13 @@ export function Outer({ setDialogIsOpen(control.id, 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( cb => { + setDialogIsOpen(control.id, false) + setIsVisible(false) + setIsOpen(false) + setIsVisible(true) + try { if (cb && typeof cb === 'function') { cb() @@ -60,13 +56,17 @@ export function Outer({ 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 +103,7 @@ export function Outer({ + onPress={handleBackgroundPress}> 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 index a3ee97a2ed..24f61a2ee1 100644 --- 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" /> From 162059af15144fd73c5b23de255d46ade4c356dc Mon Sep 17 00:00:00 2001 From: Hailey Date: Tue, 9 Apr 2024 15:02:25 -0700 Subject: [PATCH 2/2] 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 --- src/components/Dialog/index.tsx | 61 ++++++----- src/components/Dialog/index.web.tsx | 33 +++--- src/view/screens/Storybook/Dialogs.tsx | 137 ++++++++++++++++++++++++- 3 files changed, 186 insertions(+), 45 deletions(-) diff --git a/src/components/Dialog/index.tsx b/src/components/Dialog/index.tsx index 1249897b2b..55798db7f5 100644 --- 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( ({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(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}> { - 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') { - 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 && ( - - )} + - {isVisible ? children : null} + {children} 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. + + + + + + + + + + + + + + + + ) }