-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Migrate Two Factor Authentication to use separate screens (remove react-native-animatable) #54404
base: main
Are you sure you want to change the base?
Migrate Two Factor Authentication to use separate screens (remove react-native-animatable) #54404
Conversation
…ated for fade-in animations
…eaner implementation
0aaa367
to
2984009
Compare
react-native-animatable
with react-native-reanimated
react-native-reanimated
, deprecate react-native-animatable
, remove redundant useNativeDriver
…uthentication screens
react-native-reanimated
, deprecate react-native-animatable
, remove redundant useNativeDriver
Update: PR is once again ready for review! @shubham1206agra please have a look - I added some new test steps 😅 |
@shawnborton @dubielzyk-expensify A lot of things changed under the hood - 2FA steps are separate screens with different urls now, so it is more consistent with the rest of the app - and animations work out of the box. The bottom line for you is that animations are butterly smooth! 🎉 Here is a comparison: Before:web-before.movAfter:web-after.mov |
The animations are looking nice, but why is the magic code input anchored to the bottom of the screen with the button? (Maybe that's not related to this specifically?) |
The animations look great! @dannymcclain let's fire up a separate bug report for the anchored magic code input, that is indeed weird! |
Oh right - I forgot to mention that I didn't touch the input as the PR is already pretty big and I didn't want to introduce more changes. @shawnborton Could you tag me in the bug report? We have a new team member, and this looks like a great first issue for him to tackle! |
@blazejkustra just tagged you here! |
Explanation of Change
Initially I wanted to migrate animatable to reanimated in one PR but changes from AnimatedStep component become too big to migrate everything in one PR. Here is the first PR.
The entire two-factor authentication (2FA) flow is designed to work within a single screen, which meant the animation had to closely mimic how react-navigation handles transitions.
This is the main issue with current transition, it doesn't align with how other screens in the app behave. Instead of starting from a consistent position, the animation began from an arbitrary point in the middle of the screen. This resulted in a clunky animation experience. Please check what is the difference on three platforms:
Before:
web-before.mov
ios-before.mp4
android-before.webm
After:
web-after.mov
ios-after.mp4
android-after.webm
Fixed Issues
$ #52920
PROPOSAL: #52920 (comment)
Tests
Video
copilot.mov
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)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
Check
Explanation of Change
section with videos of how it looks now.