Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small logic cleanups #3451

Merged
merged 14 commits into from
Apr 9, 2024
Merged

Small logic cleanups #3451

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d7c7cab
remove a few things
haileyok Apr 9, 2024
7af74dc
oops
haileyok Apr 9, 2024
deb4b27
stop swallowing the error
haileyok Apr 9, 2024
121840c
queue callbacks
haileyok Apr 9, 2024
c9831f9
oops
haileyok Apr 9, 2024
e98ebae
log error if caught
haileyok Apr 9, 2024
84d5ceb
no need to be nullable
haileyok Apr 9, 2024
8dfc085
move isClosing=true up
haileyok Apr 9, 2024
bebdc8f
reset `isClosing` and `closeCallbacks` on close completion and open
haileyok Apr 9, 2024
4042122
run queued callbacks on `open` if there are any pending
haileyok Apr 9, 2024
9f706f0
rm unnecessary ref and check
haileyok Apr 9, 2024
082d63d
ensure order of calls is always correct
haileyok Apr 9, 2024
ef11e61
call `snapToIndex()` on open
haileyok Apr 9, 2024
7982c8f
add tester to storybook
haileyok Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 32 additions & 27 deletions src/components/Dialog/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

/*
Expand All @@ -96,28 +96,49 @@ 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} = {}) => {
setDialogIsOpen(control.id, true)
// can be set to any index of `snapPoints`, but `0` is the first i.e. "open"
setOpenIndex(index || 0)

callQueuedCallbacks()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to call all close callbacks here, or maybe just reset the an empty array on re-open?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think calling all of the old ones is the best move. I.e:

  • Press save
  • Close animation begins
  • Somehow the dialog gets opened again before the animation ends
  • ???

I think i'd expect the first thing I did to still complete personally.

},
[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(() => {
Copy link
Contributor Author

@haileyok haileyok Apr 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling this onCloseAnimationComplete I think gives us a better sense of the purpose of this function. Too many things called close and onClose gets a bit confusing I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To expand on why this is necessary:

On some lower end devices, if we try to do all of this at the same time as sheet.current?.close(), there is a significant performance drop. On Android, I can make the FPS drop to zero just by closing the sheet and removing a post from the feed at the same time.

Instead, we want to call sheet.current?.close(), and let the BottomSheet component decide when to call onCloseAnimationComplete, ensuring a smooth transition.

// 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,
() => ({
Expand All @@ -127,22 +148,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 (
Expand Down Expand Up @@ -170,7 +175,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={[
Expand Down
33 changes: 16 additions & 17 deletions src/components/Dialog/index.web.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines -47 to -49
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isVisible has a default value of true and here we never actually would have changed this to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also can't see a visual difference after removing this in the web version, so unsure if we need it. We're conditionally rendering in the dialog once isOpen changes to true, so we still get the Animated.View entering animations.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

State calls during events should be batched so I'm not sure there'd be a difference either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ya know what, this is leftovers from an exit animation we removed, def safe to remove


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)
Comment on lines +50 to +54
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good callout on this one, setTimeout gives us the expected result.

}
} catch (e: any) {
logger.error(`Dialog closeCallback failed`, {
Expand Down Expand Up @@ -113,17 +114,15 @@ export function Outer({
gtMobile ? a.p_lg : a.p_md,
{overflowY: 'auto'},
]}>
{isVisible && (
<Animated.View
estrattonbailey marked this conversation as resolved.
Show resolved Hide resolved
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={[
Expand All @@ -135,7 +134,7 @@ export function Outer({
minHeight: web('calc(90vh - 36px)') || undefined,
},
]}>
{isVisible ? children : null}
estrattonbailey marked this conversation as resolved.
Show resolved Hide resolved
{children}
</View>
</View>
</TouchableWithoutFeedback>
Expand Down
Loading