Migrate domain members list to the new Table component - #94445
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1212fc6c52
ℹ️ 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".
| if (!shouldShowGroupFilter) { | ||
| return undefined; |
There was a problem hiding this comment.
Clear stale group filter when groups disappear
When a user has selected a specific group and Onyx temporarily removes that group (for example during a rollback/refresh or after group edits), returning no filter config here hides the dropdown but does not clear the Table middleware's currentFilters. If the same group ID appears again, the old value is still stored and the members list is silently filtered back to that group; the previous hook explicitly cleared selectedGroup in this scenario to avoid reactivating stale filters.
Useful? React with 👍 / 👎.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
🚧 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.
|
Otherwise looks good to me! |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@situchan this is ready for your review. |
| shouldShowGroupColumn: boolean; | ||
| }; | ||
|
|
||
| export default function DomainMembersTableRow({item, rowIndex, shouldUseNarrowTableLayout, shouldShowGroupColumn}: DomainMembersTableRowProps) { |
There was a problem hiding this comment.
I think we're missing the brickRoadIndicator on each row, right?
There was a problem hiding this comment.
Good catch. I think we should keep the behavior consistent here.
On main, we currently have two ways to surface member errors:
- Inline error row — a full error message below the row (e.g., billing owner errors).
- Brick road dot — just a red dot on the row, with the actual error shown on the detail page (e.g., vacation delegate / 2FA exempt errors).
I think we should unify this and have all member errors use the table's inline error row, similar to Categories. That way, there's a single, consistent error pattern across the table instead of mixing inline errors and brick road indicators.
What do you think? Should we keep everything consistent with inline error rows, or do you think we should keep the brick road dot for detail-only errors?
cc: @Expensify/design
There was a problem hiding this comment.
I'd probably go with however we do it on the main Members table, which I would imagine would be the same as Categories? Curious for others' thoughts too.
There was a problem hiding this comment.
Agree with that. I think if we can consistently pull this off:
a full error message below the row
That would be ideal. It's consistent with Spend page too.
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
| const shouldShowSearchBar = members.length >= CONST.STANDARD_LIST_ITEM_LIMIT; | ||
| const shouldShowGroupFilter = !!filterConfig; | ||
| const groupOptionValuesKey = filterConfig?.group.options.map((option) => option.value).join(',') ?? ''; | ||
| const isEmpty = members.length === 0; |
There was a problem hiding this comment.
Also search input is now on next line. Not same line as group filter dropdown.
Is this approved design?
There was a problem hiding this comment.
@Expensify/design, should we show the illustration here, or should we match the other tables and simply display "No results found"?
There was a problem hiding this comment.
cc @JS00001 I forget what we decided to do here... I think maybe we decided to do something like:
- if table has truly no data, we show a generic empty state encouraging you to create something
- if search input returns no data from your search, we use that illustrated empty state above?
@Krishna2323 @shawnborton can we just keep the 'no results found' changes for now? In this issue: #94059, we're going to standardize the empty states for the tables |
|
That works for me! |
|
Any outstanding comment left? typecheck failing |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
No outstanding comments, typecheck fixed |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
@JS00001 all yours |
|
🚧 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.21-2 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.21-9 🚀
Bundle Size Analysis (Sentry): |






Explanation of Change
Fixed Issues
$ #86216
PROPOSAL:
Tests
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
android_mWeb.mp4
MacOS: Chrome / Safari
web_chrome.mp4