Add toggle for enable/disable instead of label#54451
Conversation
|
I'll review this today. |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safari54451-web.mp4 |
|
@abzokhattab Could you please fix merge conflicts? Thanks. |
|
Done! thanks for the comment...please review and let me know if you have any other comment. |
|
@abzokhattab Looks good. Could you please check and fix the lint error and then I can approve? Thanks. |
|
ok thanks working on them |
|
@abzokhattab Any update here? |
…/disable-instead-of-label
|
Fixed |
| .sort((rateA, rateB) => (rateA?.rate ?? 0) - (rateB?.rate ?? 0)) | ||
| .map((value) => ({ | ||
| value: value.customUnitRateID ?? '', | ||
| value: value.customUnitRateID ?? CONST.EMPTY_STRING, |
There was a problem hiding this comment.
No, this approach is wrong.
Please check https://github.com/Expensify/App/blob/main/contributingGuides/STYLE.md#default-value-for-inexistent-ids:
String IDs should not have a default value
This was the whole purpose of the eslint change – to correctly handle such cases.
Since this PR, apparently, caused multiple regressions, I suggest that a follow-up PR should handle these missing IDs properly.
An example PR for proper handling is #54297
There was a problem hiding this comment.
@abzokhattab @akinwale yes please, lets fix this. Please read over the documentation linked and feel free to ask in slack (also search past convos in slack as this was discussed multiple times)
There was a problem hiding this comment.
Thank you, I misunderstood that we can use the default value from a fixed constant. Should I raise a PR to fix the blocker issues along with this one?
There was a problem hiding this comment.
@abzokhattab Please raise a single PR to fix all the issues and tag the fixed issues in the OP. I am available to review so ping me when it's ready.
grgia
left a comment
There was a problem hiding this comment.
Changes LGTM, comment is NAB
|
✋ 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/grgia in version: 9.0.83-0 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.83-5 🚀
|
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.83-5 🚀
|





Explanation of Change
Fixed Issues
$ #53798
PROPOSAL: #53798 (comment)
Tests
Categories :
Taxes :
Distance Rates :
Tags :
Multilevel Tags :
Prerequisites:
Report Fields :
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)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
Screen.Recording.2024-12-12.at.00.45.35.mov
Android: mWeb Chrome
Screen.Recording.2024-12-12.at.00.51.20.mov
iOS: Native
Screen.Recording.2024-12-12.at.00.45.35.mov
iOS: mWeb Safari
Screen.Recording.2024-12-12.at.00.51.20.mov
MacOS: Chrome / Safari
Categories:
Screen.Recording.2024-12-12.at.00.52.19.mov
Taxes:
taxes.mov
Distance Rates:
distance.mov
Tags:
tags-single.mov
Multilevel Tags:
tags-mult.mov
Report Fields:
report-fields.mov
MacOS: Desktop
Screen.Recording.2024-12-10.at.00.04.15.mov