Migrate last components to reanimated #55202
Merged
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
…nunarrow_and_daseoverlat_to_reanimated_II # Conflicts: # src/components/TabSelector/TabSelectorItem.tsx # src/pages/Search/SearchTypeMenuNarrow.tsx
…, migrate base overly
sumo-slonik
changed the title
sumo-slonik
changed the title
|
@shubham1206agra 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] |
|
Is there anything here that needs to be reviewed by the design team? |
|
@shawnborton There are no user-visible changes here. It's just the completion of the optimization that removes animated and replaces it with reanimated. The only noticeable change might be performance improvements. |
|
Sounds good, thanks for confirming! |
| /* Callback to close the modal */ | ||
| onPress?: () => void; | ||
|
|
||
| /* Returns whether a modal is displayed on the left side of the screen. By default, the modal is displayed on the right */ | ||
| isModalOnTheLeft?: boolean; | ||
| }; | ||
|
|
||
| function BaseOverlay({shouldUseNativeStyles, onPress, isModalOnTheLeft = false}: BaseOverlayProps) { | ||
| function BaseOverlay({onPress, isModalOnTheLeft = false}: BaseOverlayProps) { | ||
| const styles = useThemeStyles(); | ||
| const {current} = useCardAnimation(); | ||
| const {translate} = useLocalize(); | ||
|
|
||
| return ( | ||
| <Animated.View |
There was a problem hiding this comment.
@sumo-slonik Is there a reason why this component is not being migrated to Reanimated?
There was a problem hiding this comment.
BaseOverlay also utilizes react-navigation but in this case, it involves the import:
import { useCardAnimation } from '@react-navigation/stack';
Later, we use useCardAnimation to create opacity in the styles for Animated.View to obtain the animation progress like this:
opacity: current.progress.interpolate({
inputRange: [0, 1],
outputRange: [0, variables.overlayOpacity],
extrapolate: 'clamp',
}),
Due to this integration with react-navigation, migrating this component to Reanimated is not possible.
The useReanimatedTransitionProgress() from react-native-screens could potentially address the issue on native platforms. However, on those platforms, this component appears as follows:
function Overlay() {
return null;
}
Overlay.displayName = 'Overlay';
export default Overlay;
, so migration is not necessary.
There was a problem hiding this comment.
@mountiny Do you agree with this? Asking for confirmation.
There was a problem hiding this comment.
the explanation sounds good to me
|
@sumo-slonik Can you resolve conflicts here? |
…ng-to_reanimated # Conflicts: # src/components/TabSelector/TabSelectorItem.tsx
Conflicts resolved. |
|
@shubham1206agra thanks for the review, can you please complete the testing now? thank you! |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2025-01-22.at.4.13.19.PM.movMacOS: DesktopScreen.Recording.2025-01-22.at.4.16.18.PM.mov |
shubham1206agra
approved these changes
Jan 22, 2025
|
✋ 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 staging by https://github.com/mountiny in version: 9.0.89-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.89-8 🚀
|
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.
Explanation of Change
Migration of the latest components from animated to reanimated, adding a rule to the linter that blocks animated imports, adding examples of how to create animation code with reanimated, and adding exceptions to the linter in places where migration is not possible due to external libraries being used.
Fixed Issues
$ #52920
Tests
There are no visible changes in the GUI, but you can follow this procedure:
Offline tests
No needed
QA Steps
Same as test.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2025-01-14.at.15.24.04.mov
MacOS: Desktop
Screen.Recording.2025-01-14.at.15.37.40.mov