[HOLD for #98030] Move Categories & Tags settings into the header More menu - #100293
marufsharifi wants to merge 12 commits into
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| addSeparatorBefore?: boolean; | ||
|
|
||
| /** A component rendered on the right side of the option, e.g. a Switch for an inline toggle row */ | ||
| rightComponent?: ReactNode; |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-1 (docs)
Adding a named rightComponent?: ReactNode slot prop (together with its companion shouldShowRightComponent?: boolean flag) to DropdownOption is the ReactNode-slot-prop configuration anti-pattern. The options are .map()'d and spread into PopoverMenu/MenuItem (menuItems={options.map((item) => ({...item}))} in ButtonWithDropdownMenu/index.tsx), so every new positional slot widens a config-array-driven surface: the component must know about each slot and consumers cannot compose, reorder, or wrap it. This is exactly the illustrated bad example, which uses rightComponent?: ReactNode + shouldShowRightComponent.
Prefer composing the toggle row as an explicit child/compound-component slot rather than threading arbitrary JSX through the option config. For example, expose a dedicated menu-row component the consumer renders directly instead of passing rightComponent/shouldShowRightComponent in the option object:
<ButtonWithDropdownMenu.Menu>
<ButtonWithDropdownMenu.ToggleRow
title={translate('workspace.categories.showCategoryGLCodes')}
isOn={policy?.showCategoryGLCodes ?? false}
onToggle={(value) => setPolicyShowCategoryGLCodes(policyId, value)}
/>
</ButtonWithDropdownMenu.Menu>so adding a new row type never requires expanding DropdownOption.
Reviewed at: 4927c16 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| menuItems.push({ | ||
| text: translate('workspace.categories.showCategoryGLCodes'), | ||
| value: CONST.POLICY.SECONDARY_ACTIONS.SETTINGS, | ||
| // The row itself is inert; only the Switch handles the toggle so the menu stays open. |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This comment joins two independent clauses with a semicolon. Comments should read as plain sentences, using two separate sentences instead of a semicolon.
Split it into two sentences:
// The row itself is inert. Only the Switch handles the toggle so the menu stays open.Reviewed at: 4927c16 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| menuItems.push({ | ||
| text: translate('workspace.tags.showTagGLCodes'), | ||
| value: CONST.POLICY.SECONDARY_ACTIONS.SETTINGS, | ||
| // The row itself is inert; only the Switch handles the toggle so the menu stays open. |
There was a problem hiding this comment.
❌ CONSISTENCY-16 (docs)
This comment joins two independent clauses with a semicolon. Comments should read as plain sentences, using two separate sentences instead of a semicolon.
Split it into two sentences:
// The row itself is inert. Only the Switch handles the toggle so the menu stays open.Reviewed at: 4927c16 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
@marufsharifi Could you check the comments above? |
|
@huult The feedbacks have been resolved, thanks |
| containerStyles={containerStyles} | ||
| menuItems={options.map((item, index) => ({ | ||
| ...item, | ||
| // Build the Switch here from typed data so callers pass `switchProps` instead of threading JSX through the option config. |
There was a problem hiding this comment.
@marufsharifi Do we need a comment here? I think the code is clear enough to understand without it, so we can remove it.
| onFocus={() => { | ||
| if (!shouldUpdateFocusedIndex) { | ||
| // Inert rows (e.g. an inline toggle row whose only control is a Switch) shouldn't become the | ||
| // focused/highlighted item. Otherwise focus bubbling from the inner control leaves the row highlighted. |
There was a problem hiding this comment.
@marufsharifi Do we need a comment here? I think the code is clear enough to understand without it, so we can remove it.
| // Under the revamp the other settings moved to Rules, so this is only worth showing for the GL codes toggle. | ||
| if (canWriteCategories && (!isRulesRevampEnabled || !!policy?.glCodes)) { | ||
| // Under the revamp the Settings page is gone, so its remaining GL codes toggle is surfaced directly in this | ||
| // menu instead of behind a dedicated Settings page. |
There was a problem hiding this comment.
@marufsharifi Do we need a comment here? I think the code is clear enough to understand without it, so we can remove it.
There was a problem hiding this comment.
@huult This comment already existed on main — I only updated its wording for the new placement. I'd keep it for context, but can remove if you prefer. thanks
| const menuItems = []; | ||
| if (shouldShowTagsSettings) { | ||
| // Under the revamp the Settings page is gone, so its remaining rows (custom tag name and the GL codes toggle) | ||
| // are surfaced directly in this menu using the same visibility rules the Settings page used. |
There was a problem hiding this comment.
@marufsharifi Do we need a comment here? I think the code is clear enough to understand without it, so we can remove it.
BUG: The "Auto-categorize new expenses" toggle is completely missing from Categories. |
@marufsharifi I didn't see you remove anything. Is this missing, or has it still not been removed? |
Screen.Recording.2026-09-06.at.16.04.44.movScreen.Recording.2026-09-06.at.16.06.07.movBUG: The error message is not displayed, and the toggle silently reverts when simulating a network failure. (tags and categories) |
Bug: the switch sits pinned to the top of the row instead of centered against the wrapped text. |
Screen.Recording.2026-09-06.at.16.16.16.movBUG: When I press Enter on the toggle and it redirects to edit the tag name, the toggle value should still switch. |
Screen.Recording.2026-09-06.at.16.18.50.movBUG: When I press Enter on a toggle without any action |
@huult I kept that page on purpose for now. Right now, this feature works only with the rulesRevamp beta. When the beta is on, the Settings item is removed and the rows move to the More menu. When the beta is off, I kept the Settings page and routes as a fallback. Could you please clarify the timing? Should I remove the Settings page now, or later when this feature is out of beta and enabled for everyone? thanks |
- Show the save error inside the menu row on failure instead of reverting silently: PopoverMenu now forwards each item's errors/onCloseError to its OfflineWithFeedback, and the GL-code rows pass errorFields + clearPolicyErrorField. - Make the inline toggle rows keyboard-accessible: the row is interactive and toggles on select (click or Enter), and the Switch is rendered display-only (pointerEvents="none") so the mouse path doesn't double-toggle. - Center the Switch against the (possibly wrapped) label via alignItemsCenter. - Add errors/onCloseError to DropdownOption/PopoverMenuItem so a menu row can
|
@huult To fix the keyboard bugs, the toggle row is now a normal interactive menu item, so it highlights on hover/focus like the other rows (Import, Download). REC-20260907183732.mp4@heyjennahay Is that highlight acceptable, or should the toggle rows have no highlight? I want to confirm the expected design before adding extra handling to remove it while keeping keyboard access. thanks |
@marufsharifi So, should we wait until they come back before adding “Auto-categorize new expenses” back in? |
@marufsharifi Could you double-check whether we can remove it? The requirement is that it should be removed. |
@huult I checked again — we can remove the Settings page only when the What do you think — does this work, or should I remove it fully now? Thanks! |
@huult Yes, I think we should wait until that setting is restored on main. |
…pdate WorkspaceTagsPage to include pendingAction for policy tags
@marufsharifi I think we should flag this issue, comment on the ticket, and let @JmillsExpensify decide. |
@marufsharifi If we keep the Settings page as a fallback when the beta is off, which was the original design of the PR, then I think we should create a follow-up issue to fully remove the Settings page and the fallback code once |
|
@marufsharifi please resolve the conflict |
Yes, that's sound good to create a follow-up issue to fully remove the Settings page. thanks |
|
@marufsharifi #99886 (comment) Could you update this to Hold? |
marufsharifi
changed the title
|
@marufsharifi Please keep this PR on hold until PR #100248 is completed. |
2 participants
Explanation of Change
Behind the
rulesRevampbeta, the per-page settings on the workspace Categories and Tags pages now live directly in the header More menu instead of a separateSettingsRHP:Import spreadsheet/Download CSV.When the beta is off, the existing
Settingspages are kept unchanged as a fallback.Fixed Issues
$ #99886
PROPOSAL:
Tests
Categories
Go to Workspace → Categories → More. The menu should display:
There should be no Settings option.
Turn on or off Show GL codes when categorizing expenses.
Tags
Go to Workspace → Tags → More. The menu should display:
There should be no Settings option.
Tap Custom tag name.
Turn on or off Show GL codes when selecting a tag.
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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
Categories
Screen.Recording.2026-09-04.at.6.46.40.PM.mov
Tags
Screen.Recording.2026-09-04.at.6.47.05.PM.mov
Android: mWeb Chrome
Categories
Recording_20260904_184053.mp4
Tags
Recording_20260904_184137.mp4
iOS: Native
Categories
Screen.Recording.2026-09-04.at.6.55.52.PM.mov
Tags
Screen.Recording.2026-09-04.at.6.56.16.PM.mov
iOS: mWeb Safari
Categories
Screen.Recording.2026-09-04.at.6.57.17.PM.mov
Tags
Screen.Recording.2026-09-04.at.6.58.35.PM.mov
MacOS: Chrome / Safari
Categories
Screen.Recording.2026-09-04.at.6.38.00.PM.mov
Tags
Screen.Recording.2026-09-04.at.6.41.11.PM.mov