[TS migration] Migrate 'SettingsAppDownloadLinks' page to TypeScript #25175 #35890
Conversation
…o AppDownloadLinks.tsx Updates related to Typescript Migration Expensify#25175. -Migrated component to Typescript -Extended MenuItemProps type as DownloadMenuItem type -Typed menu items as an array of DownloadMenuItems -Removed withWindowDimensions/withLocalize PropTypes and compose() from export per guidelines on migration -Used Hooks for localize instead for translation -Removed _ in JSX mapping of menu items. Saw other TSX files that removes and I was getting ESLint warnings. _ is noted as a best practice but given other TSX files have removed I followed suit. I also saw old ESlint ignores being used when removing _ but I didn't get flagged without this. -Removed onKeyDown attribute and blur() from MenuItem. I saw this nowhere else in the codebase and the removal had no impact on app use functionality (please advise if there's something I am missing but it appeared antiquated) -This component receives props from Navigation but I do not see elsewhere in the codebase that we are explicitly listing these as props via typescript (navigation props should inherintly apply to all pages) -TS warnings made me list translationKey as type TranslationPaths (warning only appears when not providing the translate from useLocalize() with a '' - others working on TS migrations may want to know this) -No ESlint warnings -No VSCode warnings or errors -No errors when testing -Only issue was on iOS simulator there's no App Store app so it doesn't know how to handle the download iOS click but the link is fine and it works everywhere else. -No other components or other files in codebase import this file so change from .js to .tsx should not require changes to imports anywhere. This component is routed to via its name as with other pages in codebase.
|
@Santhosh-Sellavel 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] |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA document and I hereby sign the CLA |
|
recheck |
|
Can you fix the lint issues? |
|
Please link the issue properly in the description. Also, can you check and update descriptions correctly there few typos |
hkopser99
changed the title
|
@Santhosh-Sellavel ran Prettier so the lint issues should be resolved. Do I need to tell the bot to recheck as I did after signing the agreement? I also fixed the description and the linking to the fix along with a few other revisions for clarity and conciseness. I found another recent PR and tried to make mine more like that so you have an easier review. If you notice anything else missing just let me know. Its my first PR for Expensify so I want to make sure you have everything you need. |
|
@Julesssss |
|
@Santhosh-Sellavel yeah done 👍 |
|
@Santhosh-Sellavel I saw an error occurred so I ran the perf-tests locally and didn't see it on my local environment. I found a few different threads in slack from Nov 2023 - Feb of this year mentioning perf-test failures where changes were unrelated to the failure. At least one was resolved just by re-running the tests. Is this an issue you've encountered before? |
| const {translate} = useLocalize(); | ||
| const popoverAnchor = useRef<View>(null); | ||
|
|
||
| type DownloadMenuItem = MenuItemProps & { |
There was a problem hiding this comment.
Please move type declaration outside of component scope
There was a problem hiding this comment.
Thanks, I'll move that here and will be sure to move type declarations outside of component scope going forward.
| title={props.translate('initialSettingsPage.aboutPage.appDownloadLinks')} | ||
| onBackButtonPress={() => Navigation.goBack()} | ||
| title={translate('initialSettingsPage.aboutPage.appDownloadLinks')} | ||
| onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_ABOUT)} |
There was a problem hiding this comment.
My logic when I was working on the change was to make the navigation destination explicit. Now that I look at it there's no benefit to this. Navigation.goBack() works as intended here and removing the explicit destination makes the code more reusable if we ever want to present the AppDownloadLinks page from another screen in the future. If approved I will amend to remove ROUTES.SETTINGS_ABOUT as an argument and will then also be able to remove the ROUTES import statement above as this was the only use of ROUTES.
There was a problem hiding this comment.
Keep original logic if possible.
|
@hkopser99 for fixing Reassure Perfomance tests just marge main to your branch |
Moved Type declaration out of component scope.
|
@Julesssss Can you rerun workflow tests |
Yep, done |
| openAppDownloadLink: () => { | ||
| Link.openExternalLink(CONST.APP_DOWNLOAD_LINKS.ANDROID); | ||
| }, | ||
| link: CONST.APP_DOWNLOAD_LINKS.ANDROID, | ||
| downloadLink: CONST.APP_DOWNLOAD_LINKS.ANDROID, |
There was a problem hiding this comment.
Any reason for renaming? action > openAppDownloadLink, link > downloadLink
There was a problem hiding this comment.
Yes, I was following my understanding of the Style document in the Naming Conventions->Event Handlers section found here. I then adjusted link to downloadLink to be more descriptive. Please let me know if 'action' and 'link' are preferred to the more descriptive names.
There was a problem hiding this comment.
original names are fine to me
| title={props.translate('initialSettingsPage.aboutPage.appDownloadLinks')} | ||
| onBackButtonPress={() => Navigation.goBack()} | ||
| title={translate('initialSettingsPage.aboutPage.appDownloadLinks')} | ||
| onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_ABOUT)} |
There was a problem hiding this comment.
Keep original logic if possible.
|
@hkopser99 I am not sure what happened with your author checklist. Large space between each line. Please check any other PRs 
Add Tests / QA Steps |
Amended openAppDownloadLink and downloadLink in DownloadMenuItem back to action and link. Returned to original, simplified logic for Navigation.goBack()
|
@situchan thanks for reviewing. I've now returned to the original logic for Navigation.goBack(), returned to original names for 'action' and 'link' in the DownloadMenuItem type, and the testing/QA steps are unhidden (they were there but accidentally commented out). I just merged with main as well. |
Reviewer Checklist
Screenshots/VideosAndroid: Nativeandroid.movAndroid: mWeb Chromemchrome.moviOS: Nativeios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
|
Hey @situchan, no rush but I think we're just waiting on the final device testing and screen captures, right? |
right |
|
Hi @hkopser99 would you mind merging |
|
@Julesssss just merged! |
|
Thanks |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
@Julesssss thank you and thanks for letting me work on this one. Had some bumps along the way getting up and running w/ the codebase/dependencies and my laptop but thankfully the actual migration piece wasn't tricky :) Do you have any more of these or other stuff to work on? Would love to do more |
|
No worries! You can find all our open issues here, but some of them will have already been assigned to contributors. |
|
Thanks, I'll look through and find something unassigned to help with |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.4.43-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.4.43-20 🚀
|
Details
-Migrated JSX component to Typescript
-Extended MenuItemProps type as DownloadMenuItem type
-Typed the MenuItems as an array of DownloadMenuItem
-Removed withWindowDimensions/withLocalize PropTypes and compose() from export statement per guidelines on migration
-Used Hooks for localize translations
-Removed underscore in JSX mapping of menu items. I saw other files converted to TSX that removed it and were getting ESLint warnings. Using the _ is noted as a best practice but given other TSX files had removed or used an ESLint ignore I followed suit. I did not get ESLint warnings, so I did not need the ignore comment.
-I removed onKeyDown attribute and blur() from MenuItem. I did not see this elsewhere in the codebase and the removal had no impact on the page’s functionality or appearance. It appeared antiquated but please advise if there is a need for it that I am missing)
-This component receives props from Navigation but I do not see elsewhere in the codebase that we are explicitly listing navigation related props when migrating to TS (nav/routing props should inherently apply to all pages presented via Navigation)
-A TS warning made me list translationKey in DownloadMenuItem as type TranslationPaths. The warning only appeared when not providing translate from useLocalize() with a string literal. Others working on TS migrations may want to be aware of this as a solution.
-No other components or other files in the codebase import this file so changing from .js to .tsx should not require revisions to import statements. This component is routed to by using its displayName and that was not changed.
-One item to note is that on the iOS simulator there is no App Store app, so it does not know how to handle the download iOS app link click (see screenshots on iOS Native and iOS mWeb for reference). The link supplied is correct, and it works everywhere else.
Fixed Issues
$ #25175
PROPOSAL:
$ #25175 (comment)
Proposed to migrate file to Typescript and convert file type to TSX following Expensify migration best practices as well as testing on all platforms to confirm page functionality remains the same.
Tests
I ran the app on each platform, confirmed each App Download Link leads to the intended URL. I also confirmed that navigation back to the About page operates as intended when leaving the AppDownloadLinks page.
Offline tests
The links work offline but will not present web pages with content or the MacOS App Download option until internet connection has been reestablished. During this time standard Safari/Chrome warnings are presented.
QA Steps
PR Author Checklist
I linked the correct issue in the
### Fixed Issuessection aboveI wrote clear testing steps that cover the changes made in this PR
I added steps for local testing in the
TestssectionI added steps for the expected offline behavior in the
Offline stepssectionI added steps for Staging and/or Production testing in the
QA stepssectionI added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e.
toggleReportand notonIconClick)I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g.
myBool && <MyComponent />.I verified that comments were added to code that is not self explanatory
I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
I verified any copy / text shown in the product is localized by adding it to
src/languages/*files and using the translation methodI verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the
Waiting for Copylabel for a copy review on the original GH to get the correct copy.I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
I verified the JSDocs style guidelines (in
STYLE.md) were followedIf a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like
Avatar, I verified the components usingAvatarare working as expected)I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If any new file was added I verified that:
If a new CSS style is added I verified that:
A similar style doesn't already exist
The style can't be created with an existing StyleUtils function (i.e.
StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like
Avataris modified, I verified thatAvataris working as expected in all cases)If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If the PR modifies the form input styles:
I verified that all the inputs inside a form are aligned with each other.
I added
Designlabel so the design team can review the changes.If a new page is added, I verified it's using the
ScrollViewcomponent to make it scrollable when more elements are added to the page.If the
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
MacOS: Desktop