[TS migration] Migrate 'Sidebar' page to TypeScript#36847
Conversation
…s-migration/Sidebar
…s-migration/Sidebar
…s-migration/Sidebar
…s-migration/Sidebar
…s-migration/Sidebar
…s-migration/Sidebar
| }, | ||
| }), | ||
| )(FloatingActionButtonAndPopoverWithRef); | ||
| const policySelector = (policy: OnyxEntry<OnyxTypes.Policy>) => |
There was a problem hiding this comment.
Add return type and reuse it
…s-migration/Sidebar
| const reportIDs = SidebarUtils.getOrderedReportIDs( | ||
| null, | ||
| chatReports, | ||
| chatReports as OnyxEntry<Record<string, OnyxTypes.Report>>, |
There was a problem hiding this comment.
I agree, let's try at least 😅
| chatReports: { | ||
| key: ONYXKEYS.COLLECTION.REPORT, | ||
| selector: chatReportSelector, | ||
| // This assertion is needed because the selector in withOnyx expects that the return type will be the same as type in ONYXKEYS but for collection keys the selector is executed for each collection item. This is a bug in withOnyx typings that we don't have a solution yet, when useOnyx hook is introduced it will be fixed. |
There was a problem hiding this comment.
There was a recent PR in onyx, maybe it resolved the issue here?
There was a problem hiding this comment.
Yes @kubabutkiewicz The Onyx PR was merged and is available on main, let's remove the comments and apply that code I suggested to you
There was a problem hiding this comment.
will do that :)
There was a problem hiding this comment.
@blazejkustra @fabioh8010 its adjusted , you can recheck
| bottom: 0, | ||
| }} | ||
| isSmallScreenWidth={false} | ||
| // @ts-expect-error - normally this comes from withCurrentReportID hoc , but here we are just mocking this |
There was a problem hiding this comment.
| // @ts-expect-error - normally this comes from withCurrentReportID hoc , but here we are just mocking this | |
| // @ts-expect-error - normally this comes from withCurrentReportID hoc, but here we are just mocking this |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #25220 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
Conflicts |
…s-migration/Sidebar
|
More conflicts |
…s-migration/Sidebar
| @@ -127,13 +126,16 @@ function getOrderedReportIDs( | |||
| // However, this code needs to be very performant to handle thousands of reports, so in the interest of speed, we're just going to disable this lint rule and add | |||
| // the reportDisplayName property to the report object directly. | |||
| // eslint-disable-next-line no-param-reassign | |||
There was a problem hiding this comment.
Can we remove this line?
There was a problem hiding this comment.
Sorry I missed that we cant remove that because in this line we are reassigning the function parameter, if we want to do that we will need to refactor this function, but also there is a comment which is explaining why we are it was done
There was a problem hiding this comment.
The next line is if (report) {, right? Seems like two lines down we have the same disable?
There was a problem hiding this comment.
yeah you are right this one is not needed it was a leftover after some debugging and didnt see that, thanks !
|
@AndrewGable looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
🤔 - Tests were passing |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
| (reportActions && | ||
| Object.values(reportActions).map((reportAction) => { | ||
| const {reportActionID, actionName, errors = [], originalMessage} = reportAction; | ||
| const decision = reportAction.message?.[0].moderationDecision?.decision; |
There was a problem hiding this comment.
Hi team, I got this error when pulling the latest main. I tried to sign-out then sign-in couldn't solve the error. It's related to this line. Do you mind checking it? Thanks
|
@hoangzinh I will fix that, are there any steps to reproduce ? |
|
@kubabutkiewicz I'm unsure, I just logged in App then App crashed. Maybe not all the |
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.4.53-0 🚀
|
|
This was reverted because of the above error, can we try again? |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 1.4.53-2 🚀
|
9 participants
Details
Fixed Issues
$ #25220
PROPOSAL: N/A
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.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.mp4
Android: mWeb Chrome
mchrome.mp4
iOS: Native
ios.mp4
iOS: mWeb Safari
msafari.14.30.19.mp4
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4