-
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
[TS migration] Migrate 'withReportAndPrivateNotesOrNotFound.js' HOC to TypeScript #34014
Conversation
…privatenotesornotfound-to-typescript # Conflicts: # src/pages/home/report/withReportAndPrivateNotesOrNotFound.js
@cubuspl42 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] |
Looks good, I left a minor 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.
While the code looks good, the PR lacks...
- QA Steps
- Tests
- Screenshots/Videos
In Tests, write "Same as QA Steps", but fill in QA Steps.
It seems that you modified the PR template by removing the whole bottom part, please don't do that.
Sorry, I'll add the missing info in the PR description 😉 |
…privatenotesornotfound-to-typescript
@cubuspl42 Missing info added! Thank you for your patience! 🙏 |
@pac-guerreiro No worries! Sadly, some more actions are required:
|
…privatenotesornotfound-to-typescript # Conflicts: # src/pages/home/report/withReportAndPrivateNotesOrNotFound.tsx
…privatenotesornotfound-to-typescript # Conflicts: # src/pages/home/report/withReportOrNotFound.tsx
…privatenotesornotfound-to-typescript # Conflicts: # src/pages/home/report/withReportOrNotFound.tsx
@pac-guerreiro Please ping me when all checks (except PR Reviewer Checklist) are passing |
@cubuspl42 Issues resolved! 😄 |
const isPrivateNotesFetchFinished = isPrivateNotesFetchTriggered && !report.isLoadingPrivateNotes; | ||
const isPrivateNotesEmpty = accountID ? _.has(lodashGet(report, ['privateNotes', accountID, 'note'], '')) : _.isEmpty(report.privateNotes); | ||
const isPrivateNotesEmpty = accountID ? !!report?.privateNotes?.[Number(accountID)].note ?? '' : isEmptyObject(report?.privateNotes); |
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.
!!report?.privateNotes?.[Number(accountID)].note ?? ''
...and what does it mean now? I'm surprised it is accepted by the compiler and linter.
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.
Good point, the ?? ''
is not necessary as it allows isPrivateNotesEmpty
to be a string
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.
as it allows isPrivateNotesEmpty to be a string
I don't think so. That expression should parse as...
(!!report?.privateNotes?.[Number(accountID)].note) ?? ''
...and !!x
should always be a boolean
. As I understand it, !!x ?? y
never evaluates to y
.
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.
Oh now I get what you meant, nvm 😅 I removed ?? ''
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.
@cubuspl42 Maybe this eslint rule would detect this - https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-unnecessary-condition.md
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.
@blazejkustra Do you think we could make use of the mentioned lint?
Could you check this out? When I tried to edit an existing note, an endless spinner shows up. I tried switching between withReportAndPrivateNotesOrNotFound-ts-issue-1-web-converted.mp4 |
…privatenotesornotfound-to-typescript
…privatenotesornotfound-to-typescript
@cubuspl42 Great catch! Found the culprit and just pushed the fix! Thanks 😄 |
…privatenotesornotfound-to-typescript # Conflicts: # src/pages/home/report/withReportOrNotFound.tsx
@pac-guerreiro Checks, mate! |
@cubuspl42 Typechecks are now passing! Thanks @VickyStash 🙏 |
@pac-guerreiro Are you sure the spinner problem is fixed? On the latest code: withReportAndPrivateNotesOrNotFound-ts-issue-2-web-converted.mp4I checked again that it works on |
@cubuspl42 Sorry, it seems I only fixed part of the problem, which was when the user already had private notes. I just applied a fix for the situation you reported when there are no private notes yet. Let me know if there's any remaining problem here and thanks for your patience! 😄 |
Let us take a step back here. As GitHub is not displaying the diff side-by-side, I'll post a screenshot from the IDE: Why is the As I understand it, TypeScript migrations should be relatively 1:1 regarding the business logic unless we have good reasons to act otherwise. I understand that it was a "fix" to the code not working (9b6db94), but can't we achieve working code with a 1:1 migration? If it is impossible or difficult, could you explain why? |
Reviewer Checklist
Screenshots/VideosAndroid: NativewithReportAndPrivateNotesOrNotFound-ts-android-compressed.mp4Android: mWeb ChromewithReportAndPrivateNotesOrNotFound-ts-android-web-compressed.mp4iOS: NativewithReportAndPrivateNotesOrNotFound-ts-ios-compressed.mp4iOS: mWeb SafariwithReportAndPrivateNotesOrNotFound-ts-ios-web-compressed.mp4MacOS: Chrome / SafariwithReportAndPrivateNotesOrNotFound-ts-web-converted.mp4MacOS: Desktop |
We did not find an internal engineer to review this PR, trying to assign a random engineer to #31925 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
@pac-guerreiro Could you also maybe merge |
…privatenotesornotfound-to-typescript
@cubuspl42 Main rebased 😄 |
@cristipaval Would you please review this PR, as you were assigned to #31925? |
Sure I will |
✋ 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 production by https://github.com/thienlnam in version: 1.4.41-12 🚀
|
Details
Fixed Issues
$ #31925
PROPOSAL:
N/A
Tests
Private notes
Offline tests
Same as tests.
QA Steps
Same as tests.
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-01-26.at.16.17.28.mp4
Android: mWeb Chrome
Screen.Recording.2024-01-26.at.15.34.25.mp4
iOS: Native
Screen.Recording.2024-01-26.at.16.35.12.mp4
iOS: mWeb Safari
Screen.Recording.2024-01-26.at.15.37.30.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-01-26.at.15.27.04.mp4
MacOS: Desktop
Screen.Recording.2024-01-26.at.15.43.30.mp4