[No QA][TS migration] Failure notifier TypeScript migration - #38678
cristipaval merged 2 commits into
Conversation
Kicu
left a comment
There was a problem hiding this comment.
I suggested one change, lgtm otherwise 👍
| const testMockSteps = { | ||
| notifyFailure: mocks.FAILURENOTIFIER__NOTIFYFAILURE__STEP_MOCKS, | ||
| }; | ||
| } as const satisfies MockStep; |
There was a problem hiding this comment.
I'm not sure if as const is needed, probably just satisfies ... is enough.
I did a quick search on the project and did not find the pattern as const satisfies ... used anywhere else
There was a problem hiding this comment.
as const enforces immutability, satisfies generalizes a type. I think it's good to use it as long as we know the content isn't supposed to change
There was a problem hiding this comment.
fair enough, then leave as is 👍
fabioh8010
left a comment
There was a problem hiding this comment.
LGTM, but @BrtqKr let's change PR title to [No QA][TS migration] Failure notifier TypeScript migration, [No QA] must come first
|
@rushatgabhane 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] |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #36136 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
Details
Fixed Issues
$ #36136
PROPOSAL:
Tests
npm run workflow-test -- -t failureNotifierOffline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.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 and/or tagged@Expensify/designso 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop