-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
fix: restore sensor animation #41103
fix: restore sensor animation #41103
Conversation
@hungvu193 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
ios/Podfile.lock
Outdated
@@ -2062,7 +2062,6 @@ DEPENDENCIES: | |||
- ExpoImageManipulator (from `../node_modules/expo-image-manipulator/ios`) | |||
- ExpoModulesCore (from `../node_modules/expo-modules-core`) | |||
- FBLazyVector (from `../node_modules/react-native/Libraries/FBLazyVector`) | |||
- "fullstory_react-native (from `../node_modules/@fullstory/react-native`)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wonder why do we have this line here 🤔
@hungvu193 seems like it might have been forgotten to remove when the full story was reverted. Can you please prioritize this review? |
Yeah sure |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-04-27.at.16.08.11.movAndroid: mWeb ChromeScreen.Recording.2024-04-27.at.16.13.43.moviOS: Nativeios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safarichrome.movMacOS: Desktopdesktop.mov |
I got some android build issue and working on it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
We did not find an internal engineer to review this PR, trying to assign a random engineer to #40994 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
can you confirm that the changes made in this PR are to fix this one? There seems to be two associated issues: #40994 and #40890 also, @WoLewicki, could you link the library PR that made changes? |
Yeah |
We have some conflicts here @WoLewicki |
@hungvu193 since f72d330#diff-8dfec09d6e3465e7396906f31ef6078ebe1fef9e6d151260c7be2634c25d9498 has been merged, this PR only restores the background animation 🚀 |
Awesome! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super comfortable with the animation calculation logic but the usage of
Reanimated library looks correct to me 👍
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to production by https://github.com/marcaaron in version: 1.4.71-6 🚀
|
Details
PR restoring the sensor animation in the background on Report. This PR no longer bumps
react-native-live-markdown
library since it has been bumped in other PR already. The newest version of that library includes fix with correct handling of state which should improve the performance of the library and make the selection behave correctly (see #40994).Fixed Issues
$ #40994
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
androidmarkdown.mov
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop