-
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
[TS migration] Migrate Signin to TypeScript #35294
Conversation
37c3af9
to
76bd80e
Compare
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.
Partial review
/** Whether the content is visible. */ | ||
isVisible: boolean; | ||
isVisible?: boolean; |
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.
This should be kept as required. There is another PR dealing with this rn #35404 (comment). Writing a comment so I don't forgot to check again
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 actually used the same props as that PR for this component.
Reorganized code by introducing shared types in `SignInPageLayoutProps`, which streamlined the props of all components in the SignInPageLayout. The shared types are now used in Footer, SignInPageContent, SignInPageLayout, SignInHeroCopy, SignInPageHero, and SignInHeroImage components. Files have been moved to the SignInPageLayout directory for better organization.
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.
Huge effort! Looks good typescript-wise 😄
Code looks good! Will test this asap! |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemweb-chrome.moviOS: Nativeios.moviOS: mWeb Safarimweb-safari.movMacOS: Chrome / Safariweb.mov |
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25224 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
cc @puneetlath |
Is this waiting on me to review? Weird that I didn't get assigned. In any case, looks like there are a bunch of conflicts. @fedirjh can you address them and then I'll review. |
# Conflicts: # src/pages/signin/LoginForm/BaseLoginForm.tsx # src/pages/signin/SignInHeroImage.js # src/pages/signin/SignInPage.tsx # src/pages/signin/SignInPageLayout/BackgroundImage/index.ios.js # src/pages/signin/SignInPageLayout/Footer.tsx # src/pages/signin/SignInPageLayout/SignInPageContent.tsx # src/pages/signin/SignInPageLayout/index.tsx # src/types/onyx/Form.ts
@puneetlath @s77rt Merged main and resolved all the conflicts. Ready for review. |
Good job, but man that was a big PR 😅. Any way we could break this sort of thing up more in the future? |
✋ 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/puneetlath in version: 1.4.42-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.42-5 🚀
|
Details
Fixed Issues
$ #25224
PROPOSAL: #25224 (comment)
Tests
Test 1: SAML enabled but not required
Test 2: SAML enabled and required
Test 3: Login / Signup
Test 4: Email delivery failure
NVP.set('expensify_emailDeliveryFailure', {'lastSMTPErrorDatetime': moment.now().addDays(1).toString()});
NVP.set('expensify_emailDeliveryFailure', null);
in the JS console to unblock your accountTest 5: Web Only
Another Login Page is Open
view is shown in the first window/tab.Test 6: Footer Links
Offline tests
QA Steps
Same as the Tests section above + :
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.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 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-02-07.at.4.35.47.PM.mov
Screen.Recording.2024-02-07.at.4.37.56.PM.mov
Screen.Recording.2024-02-07.at.4.41.12.PM.mov
Android: mWeb Chrome
Screen.Recording.2024-02-07.at.4.28.14.PM.mov
Screen.Recording.2024-02-07.at.4.31.35.PM.mov
Screen.Recording.2024-02-07.at.4.32.25.PM.mov
iOS: Native
Screen.Recording.2024-02-07.at.3.54.29.PM.mov
Screen.Recording.2024-02-07.at.3.55.49.PM.mov
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-07.at.15.50.43.mp4
iOS: mWeb Safari
Screen.Recording.2024-02-07.at.4.02.31.PM.mov
Screen.Recording.2024-02-07.at.4.05.19.PM.mov
Screen.Recording.2024-02-07.at.4.06.20.PM.mov
MacOS: Chrome / Safari
CleanShot.2024-02-07.at.14.58.00.mp4
CleanShot.2024-02-07.at.14.59.49.mp4
CleanShot.2024-02-07.at.15.04.59.mp4
Screen.Recording.2024-02-07.at.5.10.54.PM.mov
Screen.Recording.2024-02-07.at.5.18.42.PM.mov
MacOS: Desktop
CleanShot.2024-02-07.at.15.22.52.mp4
Screen.Recording.2024-02-07.at.3.24.42.PM.mov
Screen.Recording.2024-02-07.at.3.32.07.PM.mov