Repository navigation
Rules revamp: Standardize in-line delete experience - #100570
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🚧 shawnborton has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Feels great to me 👍 |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
Looks good to me too 👍 |
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
LGTM, nice change
JmillsExpensify
left a comment
There was a problem hiding this comment.
Looks great to me as well! @situchan you are up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61eb5e0095
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Bug?: Delete lands on the wrong screen in the category-scoped flow
(Same happens in Require-fields page) bug.mov |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
🤖 Web test steps verification results: Verified the new trashcan delete header and its confirmation end-to-end, including the offline flow, through the merchant rule editor — which wires up
Code reviewNo blocking issues. The only failing check is Two nits:
Two things I checked that hold up:
One gap: the author checklist claims unit tests were added, but the PR adds none, and there is no coverage anywhere for Worth flagging separately: the CI test account should be provisioned with the view run · no recording available |
|
Above comments are not blocker though |
|
🤖 Web test steps verification results: One regression found: the new header trashcan is not gated on the The delete affordance changes outside the beta
The other three editors are safe — What makes this look unintended rather than deliberate: the reset/clear button in the very same header IS gated, at Adding Ruled out
view run · no recording available |
…elete Signed-off-by: krishna2323 <belivethatkg@gmail.com>
99d2b21
|
🚧 JS00001 has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/JS00001 in version: 9.4.75-0 🚀
|
|
Yes — one help site article needed updating. Draft PR: #100755 Why only one. This PR moved rule deletion from a labelled Delete rule button at the bottom of each editor to a trashcan icon in the header. For merchant rules, category tax rules, field requirements, flag for review, and card restrictions, the trashcan is gated behind the Agent rules are the exception. What changed in the docs.
Articles reviewed and left unchanged
The revamp tabs (Expense defaults, Field requirements, Flag for review, Card restrictions) are not documented on the help site yet, so there was nothing to update for those rule types. Once Live UI verification. I drove the web app to check labels. The Rules page confirmed the section names — Expenses, Merchant, and Agent rules (Beta badge) — which is where the Agent Rules casing fix came from. I could not open a saved rule to photograph the trashcan itself: the drive hit an unintended navigation off the dev app and I stopped rather than risk the session. The delete copy in the PR comes from source instead — @Krishna2323, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR view run · no recording available |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |















Explanation of Change
Fixed Issues
$ #100568
PROPOSAL:
Tests
Prereqs: Control workspace, account have access to
rulesRevampbeta, Categories and Taxes enabled.Delete works on all five rule types
Trashcan is hidden when there is nothing to delete
Read-only access
Offline tests
QA Steps
Same as tests
Verify that no errors appear in the JS console
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)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_hybrid.mp4
Android: mWeb Chrome
android_mWeb.mp4
iOS: Native
ios_hybrid.mp4
iOS: mWeb Safari
ios_mWeb.mp4
MacOS: Chrome / Safari
web_chrome.mp4