Add Enabled/Disabled status filter to multi-layer tags table#97048
Add Enabled/Disabled status filter to multi-layer tags table#97048MelvinBot wants to merge 3 commits into
Conversation
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
|
@MelvinBot review |
|
There's nothing for me to fix here — no code-related check is failing. Only failing check: All substantive checks are green, including
The |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
ReviewThe change is correct and minimal — it wires the shared Verified:
Minor (non-blocking):
Overall this looks good to me. The |
| }; | ||
|
|
||
| const isItemInFilter: IsItemInFilterCallback<WorkspaceTagTableRowData> = (item, filterValues) => { | ||
| if (!filterValues || filterValues.length === 0 || filterValues.includes('all')) { |
There was a problem hiding this comment.
@MelvinBot "all" filter is needed? Asking because I don't see this in UI. Only "Reset" button.
There was a problem hiding this comment.
You're right — it's not needed. The SINGLE_SELECT filter has no "All" option in the UI; clearing is done via the Reset button, which sets filterValues to an empty array, already handled by the filterValues.length === 0 check above. Nothing ever emits an 'all' value, so that branch was dead code (I'd copied it from WorkspaceCompanyCardsTable's defensive pattern).
Removed it in f9808f6.
|
The only failing checks are the PR Author Checklist and PR Reviewer Checklist — every actual CI check (tests, build, ESLint, typecheck, Storybook, perf, etc.) is passing. ✅ These two "checklist" checks aren't code or CI failures I can fix by pushing a commit. They fail by design and stay red until a human completes them:
So no code change is needed — the branch itself is green. Once the checklist boxes are filled in, these will pass. |
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
@MelvinBot add unit test |
|
Nothing has changed since my last comment — the only two red checks are still the PR Author Checklist and PR Reviewer Checklist. Every real CI check (tests, build, ESLint, typecheck, Storybook, perf, spellcheck) passes; the ones showing as pending right now are just re-running after the retrigger, not failing. These two checklist checks aren't code/CI failures and can't be fixed with a commit — they stay red until a human completes them:
So there's no fix for me to push — the branch is green. |
|
Still the same situation — the only failing checks are PR Author Checklist and PR Reviewer Checklist. Every real CI check (tests, build, ESLint, typecheck, Storybook, etc.) is green; the perf tests are still running, not failing. There's no code fix I can push here. These two checks fail by design and only turn green when a human completes them:
The branch itself is healthy — no commit needed from me. |
DetailsExplanation of ChangeAdds an Enabled / Disabled status filter to the multi-layer tags per-list view ( The shared
Fixed Issues$ #96965 Tests
Offline testsSame as Tests QA StepsSame as Tests
PR Author Checklist
@MelvinBot please set the PR body to the content in the details section above |
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Added unit tests for the Status filter in Coverage — renders the real
Notes
The human co-author still owns the |
|
Re-checked after the latest commit ( The only red check is the PR Author Checklist, which is on MelvinBot's ignore list. It's not a code/CI failure I can fix by pushing — it stays red by design until the checklist items in the PR description are ticked. Once those boxes are checked, it'll go green. So there's no fix to push here; the branch is otherwise green. |
|
✅ Updated the PR body with the content you provided. |
|
@MelvinBot mark as ready for review |
|
@mkhutornyi 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] |
|
Done — marked PR #97048 as ready for review. 🎀 One heads-up: the |
|
Still the same situation: the only failing check is the PR Author Checklist, which isn't a code or CI failure I can fix with a commit. Every real check (tests, build, ESLint, Oxfmt, typecheck, Storybook, perf) is passing. ✅ The Author Checklist stays red until the checklist items in the PR description's Author Checklist section are ticked. There's no code change to push here — please complete those boxes and the check will go green. |
DetailsExplanation of ChangeAdds an Enabled / Disabled status filter to the multi-layer tags per-list view ( The shared
Fixed Issues$ #96965 Tests
Offline testsSame as Tests QA StepsSame as Tests
PR Author Checklist
@MelvinBot please set the PR body to the content in the details section above |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
✅ Updated the PR description to the content you provided. |
JmillsExpensify
left a comment
There was a problem hiding this comment.
LGTM at a high level, though we should get screen recordings in this PR.
|
Screen recordings were added in Reviewer Checklist |
Explanation of Change
Adds an Enabled / Disabled status filter to the multi-layer tags per-list view (
WorkspaceViewTagsTable).Previously this table only had a free-text "Find tag" search, so admins with long tag lists had no way to narrow the table down to just enabled or just disabled tags.
The shared
Tablecomponent already supports dropdown filtering out of the box via thefilters(FilterConfig) +isItemInFilterprops (the same mechanism Members and Company Cards use). This table simply never wired them up. Each row already carries anenabled: boolean, so no upstream data changes were needed — this is a pure UI wiring change:statusfilter config (Enabled / Disabled) and anisItemInFilterpredicate that matches rows againstitem.enabled.filtersandisItemInFilterto the existing<Table>. The filter trigger and selected-filter chip render automatically inside theFilterBarthat was already there.shouldShowEnabledColumn(= !hasDependentTags), so it is not offered for dependent multi-layer tags — those intentionally have no per-tag enabled column/toggle. This matches the confirmed design decision on the issue.Fixed Issues
$ #96965
PROPOSAL: #96965 (comment)
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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./** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)