Gate DomainGroupsTable search bar behind a 15-item minimum#96157
Gate DomainGroupsTable search bar behind a 15-item minimum#96157MelvinBot wants to merge 1 commit into
Conversation
Co-authored-by: truph01 <truph01@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot mark this pr as ready |
melvin-bot
Bot
requested review from
trjExpensify
and removed request for
a team
|
🤖 Done — this PR is now marked as Ready for review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /** Label and accessibility label for the search input. */ | ||
| label: string; | ||
|
|
||
| /** The minimum number of items (before searching/filtering) required to render the search bar. Defaults to 0 so the search bar always shows. */ |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-1 (docs)
The new minItemsForSearchBar prop is configuration that gates whether an internal UI element renders: its sole effect is the conditional {shouldShowSearchBar && <TableSearchBar label={label} />} block below. Table is already a compound/composition component (Table.FilterBar, Table.Header, Table.Body, Table.NoResultsState), so search-bar presence should be expressed compositionally (the consumer includes or omits the search bar) rather than by threading a numeric threshold prop that branches inside the component. Each new threshold/flag prop enlarges the component's API and couples every consumer to its internals.
Suggested fix: expose the search bar as a composable child (e.g. Table.SearchBar) that the consumer renders or not, and let DomainGroupsTable decide whether to include it based on the item count, instead of adding minItemsForSearchBar to TableFilterBar:
// DomainGroupsTable
<Table.FilterBar label={translate('domain.groups.findGroup')}>
{groups.length >= CONST.DOMAIN_GROUPS_SEARCH_BAR_THRESHOLD && <Table.SearchBar label={translate('domain.groups.findGroup')} />}
</Table.FilterBar>Reviewed at: daab196 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
|
I looked into the failing checks. There's no code failure to fix — every real CI check is green. The only red check is PR Author Checklist / checklist, and it's failing for a non-code reason:
That check just verifies the author checklist boxes are ticked and the PR body is filled out. Right now:
These are things only you (the author) should complete — I won't fill out test/QA steps or tick author-checklist boxes on your behalf. Once you fill in the Tests/QA sections and check the boxes, this check will pass. For reference, everything else is passing: ESLint, typecheck, all 8 test jobs, Oxfmt, Bun tests, Storybook, React Compiler compliance, unused-styles/knip, spellcheck, and both dev builds. I also ran So nothing to push here — the ball is in your court to complete the checklist. |
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
Why 15? Isn't the magic number 12? (CC: @Expensify/design this one again 😆)
|
Re-checked — still no code failure to push. The two red checks are both human checklist checks, not CI/code failures:
Both are completed by people (you as author, and the reviewer), and I won't fill out test/QA steps or tick checklist boxes on anyone's behalf. Once those boxes are checked and the Tests/QA sections filled in, they'll go green. Every actual CI check is passing: ESLint, typecheck, all 8 test jobs, Oxfmt, Bun tests, Storybook, React Compiler compliance, knip/unused-styles, spellcheck, and both dev builds. Nothing for me to fix here. |
|
Haha I do think we settled on 12 at some point? |
|
BUT with our new table style, we wanted to always show show the search input above the table no matter what. cc @JS00001 for confirmation |
|
Ohhh, in which case, this would not be a bug? 😅 |
|
Correct! I say we close it all up 🪡 |
|
Donezo |
|
Agreed, thanks |
5 participants
Explanation of Change
The Domain → Groups table search bar was showing even when there were only a couple of groups, regressing the previous behavior where the search field only appeared once a domain had a meaningful number of groups.
This restores a minimum-item gate as an opt-in, per-table prop so no other list is silently affected:
minItemsForSearchBarprop to the sharedTable.FilterBar(src/components/Table/TableFilterBar/index.tsx), defaulting to0, so the search bar keeps showing everywhere else exactly as before.TableSearchBarnow only renders whenoriginalDataLength >= minItemsForSearchBar.nullso it doesn't leave an empty padded gap (this is a no-op for every table using the default threshold of0, where the search bar always renders).DomainGroupsTablepassesminItemsForSearchBar={CONST.DOMAIN_GROUPS_SEARCH_BAR_THRESHOLD}(15), so the search field only appears once a domain has at least 15 groups.Fixed Issues
$ #95555
PROPOSAL: #95555
Tests
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari