-
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
chore: migrate AuthScreens from withOnyx to useOnyx #52417
base: main
Are you sure you want to change the base?
chore: migrate AuthScreens from withOnyx to useOnyx #52417
Conversation
@c3024 Ready for review |
} | ||
|
||
return ( | ||
<AuthScreensMemoized |
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 see we usually fetch the data with useOnyx
directly in the main function. This way of using another parent and passing the Onyx fetched values as props is new to me. Any benefit of doing this vis-a-vis fetching these values directly with useOnyx
in the existing main AuthScreens
function?
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 have followed this approach, @blazejkustra Has explained pretty neat here.
#49103 (comment)
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.
Thanks! Missed that comment.
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.
Let's test it on all platforms, especially signing in and off. Code looks good 😄
Co-authored-by: Błażej Kustra <[email protected]>
Logging out and logging back into an account navigates to the Profile page instead of LHN and Report Central Pane. This is fine on staging. profilePageOpensOnLogin1.mp4profilePageOpensOnLogin.mp4 |
@BhuvaneshPatil any thoughts on the above comment? |
I spent some time on that not able to figure out what is root cause |
@BhuvaneshPatil Are you saying we need to close this PR and re-open the GH for other proposals? |
@BhuvaneshPatil if you could please let us know ^ |
@deetergp We can pull current C+ to take a closer look. |
@c3024 Do you have any thoughts on this? |
Can you please help here? With this suggestion, if we logout and log back into an account it navigates to Profile Settings page instead of a Report page as mentioned here. I see that we found two regressions with the previous PR #49185 I think the previous PR #49185 can be modified to fix these like (1) using a const isAnyOnyxValueStillLoading = isLoadingOnyxValue(sessionStatus, lastOpenedPublicRoomIDStatus, initialLastUpdateIDAppliedToClientStatus);
const areAllOnyxValuesLoadedFirstTime = useRef(true);
useEffect(() => {
if (!areAllOnyxValuesLoadedFirstTime.current || !isAnyOnyxValueStillLoading) {
return;
}
areAllOnyxValuesLoadedFirstTime.current = false;
.....
}, [isAnyOnyxValueStillLoading]); |
Can you try the changes specified above and see if you find any issue? |
Explanation of Change
migrate AuthScreens component to have useOnyx instead of withOnyx
Fixed Issues
$ #49103
PROPOSAL: #49103 (comment)
Tests
This is migration task
Verify if all the flows on app same as before
Test if loading spinner is shown
Test removing report id from url and if correct report id is shown
Verify that no errors appear in the JS console
Offline tests
Same as above, migration task. App should behave as it was
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
same as above
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
Android: Native
Screen.Recording.2024-11-13.at.7.26.39.PM.mov
Android: mWeb Chrome
Screen.Recording.2024-11-14.at.9.54.23.PM.mov
iOS: Native
Screen.Recording.2024-11-13.at.6.49.57.PM.mov
iOS: mWeb Safari
Screen.Recording.2024-11-14.at.9.45.30.PM.mov
MacOS: Chrome / Safari
Screen.Recording.2024-11-13.at.7.28.46.PM.mov
MacOS: Desktop
Screen.Recording.2024-11-13.at.7.34.26.PM.mov