fix: Book travel - Book travel animation becomes blank while RHP is dismissed #50199
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
6 tasks
suneox
reviewed
Oct 4, 2024
src/components/Lottie/index.tsx
Outdated
| @@ -79,19 +79,20 @@ function Lottie({source, webStyle, shouldLoadAfterInteractions, ...props}: Props | |||
| return unsubscribeNavigationBlur; | |||
| }, [browser, navigationContainerRef, navigator]); | |||
|
|
|||
| // If user is being navigated away, let pause the animation to prevent memory leak. | |||
| useEffect(() => { | |||
| if (!animationRef || !hasNavigatedAway) { | |||
There was a problem hiding this comment.
animationRef always an object
Suggested change
| if (!animationRef || !hasNavigatedAway) { | |
| if (!animationRef.current || !hasNavigatedAway) { |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-10-04.at.15.13.22.mp4Android: mWeb ChromeScreen.Recording.2024-10-04.at.15.15.38.mp4iOS: NativeScreen.Recording.2024-10-04.at.14.39.34.mp4iOS: mWeb SafariScreen.Recording.2024-10-04.at.14.44.58.mp4MacOS: Chrome / SafariScreen.Recording.2024-10-04.at.14.33.01.mp4MacOS: DesktopScreen.Recording.2024-10-04.at.14.34.21.mp4 |
|
@layacat We need to retain the current forwardedRef. I’ve provided the necessary implementation code, so you can proceed with updating the changes accordingly. Please provide feedback if you encounter any issues during the update. |
I didn't find any usages of the forwardRef, do we need to keep it? |
@layacat Currently we have many issues being implement, which maybe involve using ref, so we don’t have the context to remove the forwardRef and keep forwardRef doesn't break the functionality |
|
If we add forwardRef back, we also need to pass the ref everywhere we used Lottie (or maybe where we faced the issue aka the BookTravel - RHP). What do you think? |
@layacat We have two refs in this case: one is the forwardRef, and the other is an internal ref called animationRef. ref={(ref) => {
if (typeof forwardedRef === 'function') {
forwardedRef(ref);
} else if (forwardedRef && 'current' in forwardedRef) {
forwardedRef.current = ref; // this is an external ref
}
animationRef.current = ref; // this is an internal ref will handle pause animation
}} |
|
I see. Thanks for the advice. Let me update the PR. |
|
updated @suneox |
suneox
approved these changes
Oct 7, 2024
src/components/Lottie/index.tsx
Outdated
| @@ -100,7 +101,7 @@ function Lottie({source, webStyle, shouldLoadAfterInteractions, ...props}: Props | |||
| // eslint-disable-next-line react/jsx-props-no-spreading | |||
| {...props} | |||
| source={animationFile} | |||
| ref={ref} | |||
| ref={animationRef} | |||
There was a problem hiding this comment.
Please keep current forward ref
Suggested change
| ref={animationRef} | |
| ref={(ref) => { | |
| if (typeof forwardedRef === 'function') { | |
| forwardedRef(ref); | |
| } else if (forwardedRef && 'current' in forwardedRef) { | |
| // eslint-disable-next-line no-param-reassign | |
| forwardedRef.current = ref; | |
| } | |
| animationRef.current = ref; | |
| }} |
src/components/Lottie/index.tsx
Outdated
| @@ -18,7 +17,8 @@ type Props = { | |||
| shouldLoadAfterInteractions?: boolean; | |||
| } & Omit<LottieViewProps, 'source'>; | |||
|
|
|||
| function Lottie({source, webStyle, shouldLoadAfterInteractions, ...props}: Props, ref: ForwardedRef<LottieView>) { | |||
| function Lottie({source, webStyle, shouldLoadAfterInteractions, ...props}: Props) { | |||
| const animationRef = useRef<LottieView>(null); | |||
There was a problem hiding this comment.
Allow nullable for reassign ref
Suggested change
| const animationRef = useRef<LottieView>(null); | |
| const animationRef = useRef<LottieView | null>(null); |
|
@layacat any specific reason to force push the commits? It is not really preferred as it removes commit history and makes reviewing (following conversation and code changes) bit challenging |
MonilBhavsar
reviewed
Oct 7, 2024
Sorry, I'm familiar with rebase flow (my company used it as the mandatory rule) so I did it as a habit. I will try to do merge flow from now then. |
MonilBhavsar
reviewed
Oct 8, 2024
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
58 tasks
|
🚀 Deployed to staging by https://github.com/MonilBhavsar in version: 9.0.47-1 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.47-4 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #49744
PROPOSAL: #49744 (comment)
Tests
Offline tests
Same with Tests
QA Steps
Same with Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.native.mov
Android: mWeb Chrome
android.web.mov
iOS: Native
iphone.native.mov
iOS: mWeb Safari
iphone.web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov