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 2 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
47 changes: 22 additions & 25 deletions src/components/Dialog/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -105,19 +104,33 @@ 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<DialogControlProps['close']>(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()
}, [])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we actually care if the callback changes? Or can it? From what I can tell if we call close then the dialog's lifetime is going to end and there won't be another chance for this to get called. Unless we are worried about control.close() getting called multiple times for some reason?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In principle we should take multiple calls into account since there's no mechanism to prevent those, and if they happen in different parts of the codebase (which at first might be independent and then due to another change might start firing together), each of them may have an expectation that the callback will fire.

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.

Okay. Perhaps I am misunderstanding what is going on outside of here, but I would imagine that we wouldn't actually hit this case. Let me look some more. I'd rather that just not be a possibility at all.

Clarified the intention in Slack


// 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)

try {
if (closeCallback.current) {
closeCallback.current()
}
} catch (e) {
// No need to handle this here
} finally {
onClose?.()
}
}, [control.id, onClose, setDialogIsOpen])

useImperativeHandle(
control.ref,
() => ({
Expand All @@ -127,22 +140,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 +167,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
27 changes: 11 additions & 16 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,20 +33,17 @@ 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') {
Expand Down Expand Up @@ -113,17 +110,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 +130,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