Small logic cleanups #3451
Small logic cleanups #3451
Conversation
| setIsVisible(false) | ||
| setIsOpen(false) | ||
| setIsVisible(true) |
There was a problem hiding this comment.
isVisible has a default value of true and here we never actually would have changed this to false.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
State calls during events should be batched so I'm not sure there'd be a difference either way.
There was a problem hiding this comment.
Oh ya know what, this is leftovers from an exit animation we removed, def safe to remove
| 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(() => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/components/Dialog/index.tsx
Outdated
| // 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() | ||
| }, []) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 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) |
There was a problem hiding this comment.
good callout on this one, setTimeout gives us the expected result.
src/components/Dialog/index.tsx
Outdated
| 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() |
There was a problem hiding this comment.
Do we want to call all close callbacks here, or maybe just reset the an empty array on re-open?
There was a problem hiding this comment.
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.
* Small logic cleanups * 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 --------- Co-authored-by: Hailey <[email protected]>
Test Plan
Open Testerto see the various edge cases and watch the console logs.