fix: add explicit None option to rule selection pickers#90914
Conversation
Place the None option after the currently selected item to preserve the focused/highlighted state and scroll position of the selected item.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| initiallySelectedItem: undefined, | ||
| }); | ||
|
|
||
| const noneItem = screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}__none_option__`); |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The string '__none_option__' is a magic value duplicated from the NONE_OPTION_KEY constant defined in SearchSingleSelectionPicker.tsx. If the constant value changes, this test will silently break.
Export NONE_OPTION_KEY from SearchSingleSelectionPicker.tsx and import it in the test:
import SearchSingleSelectionPicker, { NONE_OPTION_KEY } from '@components/Search/SearchSingleSelectionPicker';
// Then use it:
const noneItem = screen.getByTestId(`${CONST.BASE_LIST_ITEM_TEST_ID}${NONE_OPTION_KEY}`);Reviewed at: 28bdbe6 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28bdbe66ce
ℹ️ 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".
| keyForList: NONE_OPTION_KEY, | ||
| isSelected: !selectedItem?.value, |
There was a problem hiding this comment.
The synthetic option uses a hard-coded keyForList (__none_option__), but regular rows also use item.value as keyForList; in rule pickers those values come from user/workspace data. If a category/tag/tax identifier equals __none_option__, the list will contain duplicate keys/IDs for two different rows, which can cause incorrect row reconciliation/selection behavior and ambiguous testID targeting. Use a reserved key that cannot collide with user data (or derive a guaranteed-unique internal key) for the synthetic row.
Useful? React with 👍 / 👎.
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Nice, good change!
…tion-rule-selection
- Change NONE_OPTION_KEY to use null byte prefix, preventing collisions with user-created category/tag/tax identifiers - Export NONE_OPTION_KEY and import it in tests instead of duplicating the magic string
|
@Expensify/design When there is a selected item, which one should have the priority to be at the top - 90914-query-001.mp4 |
Hmm I think @mkhutornyi is working on this but I thought the idea was:
So then in the case of the list having 12 or more options, I would think None would show up after the selected rows. Definitely curious what the rest of @Expensify/design is thinking though, I don't feel strongly here. |
|
Yeah that sounds right to me. The details being that it doesn't move to the top when it's clicked, but moves to the top when you come back into the menu. So on the actual selection it stays in place, right? |
|
Yup exactly! |
|
Yup that sounds good to me too 👍 |
|
@rojiphil Addressed the NAB comment. Moved Regarding the position of the None option: the current implementation already matches the design expectation. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp90914-android-hybrid-001.mp4Android: mWeb Chrome90914-mweb-chrome-001.mp4iOS: HybridApp90914-ios-hybrid-001.mp4iOS: mWeb Safari90914-mweb-safari-001.mp4MacOS: Chrome / Safari90914-web-chrome-001.mp4 |
|
Tagging @shawnborton for design review |
|
Generally looks good to me. One comment though - are you changing the behavior of what and when we put a selected item at the top of a list? If I recall, I think we decided that we always want the selected item to float to the top of the list ONLY IF the list is long enough to scroll off of the page. Or maybe ONLY IF we have greater than or equal to X number of items. That might be something we're handling elsewhere, but just wanted to call it out. Otherwise this situation gets a bit odd: cc @Expensify/design in case you have thoughts on that |
Yeah @shawnborton. That seems to be addressed somewhere else as you had mentioned in #90914 (comment). |
Oh I definitely do haha, but it sounds like it's being handled elsewhere 👍
The |
|
Okay sounds good to me! cc @mkhutornyi for visibility |
|
Agree with everything said by Danny and Shawn. Looking good otherwise 👍 |
|
🚧 @MariaHCD has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ 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/MariaHCD in version: 9.3.97-0 🚀
Bundle Size Analysis (Sentry): |
Help site review — docs update requiredYes — this PR introduces user-facing behavior that wasn't documented. Adding the None option lets users clear a previously selected Category, Tag, or Tax value when configuring a rule, and since
Neither article previously explained how to clear a field value, so I added a concise FAQ entry to each. Draft help site PR: #92456 UI labels were verified against Note I couldn't set you as the assignee on the draft PR — GitHub only allows assigning users with write access or who have commented on that PR. Please self-assign (or comment once and I can retry). The @aswin-s, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.97-1 🚀
|
Adds FAQ entries to Workspace Merchant Rules and Personal Expense Rules explaining how to clear a Category, Tag, or Tax value using the new None option, reflecting Expensify#90914. Co-authored-by: Aswin S <aswin-s@users.noreply.github.com>
9 participants
Explanation of Change
When editing a merchant rule (tag, category, or tax), users had no explicit way to clear a previously selected value. The only method was clicking the selected option again, which was undiscoverable.
This PR adds an explicit "None" option to the
SearchSingleSelectionPickercomponent, gated behind a newallowNoneOptionprop. When enabled, a localized "None" item appears in the selection list after the currently selected item, allowing users to clear their selection with a single tap. TheonSelectItemguard is also updated from!item.valuetoitem.value === undefinedso the empty-string value of the None sentinel is not silently dropped.RuleSelectionBasepassesallowNoneOptionto enable this for all six rule selection pages (AddTagPage, AddCategoryPage, AddTaxPage in both MerchantRules and settings/Rules). Other callers likeSearchFiltersCurrencyBaseare unaffected since the prop defaults tofalse.Fixed Issues
$ #89536
PROPOSAL: #89536 (comment)
Tests
Offline tests
QA Steps
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))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-native.mp4
Android: mWeb Chrome
android-web.mp4
iOS: Native
ios-native.mp4
iOS: mWeb Safari
ios-web.mp4
MacOS: Chrome / Safari
MacOS.mp4