Migrate group members page to the new Table layout#95224
Conversation
Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
I created this PR from an upstream branch since To take ownership of this branch and push updates from your own fork, run: Then you can continue the work from your fork (open a new PR from it if you prefer). |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot selection is not working at all (no checkbox in web, nothing happens on long-press mobile) |
Gate the Table selection UX (checkbox visibility, long-press, and mobile selection-mode auto-sync) on isSmallScreenWidth instead of shouldUseNarrowLayout, matching the old SelectionListWithModal. shouldUseNarrowLayout is always true inside a right-hand panel regardless of screen size, so selection was fully disabled there. Visual layout still uses shouldUseNarrowLayout, so this is a no-op for central-pane tables (where the two flags are equal) and only enables selection in the RHP. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
Fixed and pushed ( Root cause
The old Fix — key the selection UX (checkbox visibility, long-press, and the mobile selection-mode auto-sync) off Result:
Why this is safe for existing (central-pane) tables
Verification: |
|
@codex review |
|
@MelvinBot review |
ReviewOverall this is a clean migration that closely mirrors Correctness (checked against the code):
Nits (optional):
Shared QA / behavior changes to confirm on device (all intentional per the description):
Local checks I could run
I'm not approving (that's for a human reviewer) — this is an automated review pass. |
|
Codex Review: Didn't find any major issues. 🎉 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". |
Explanation of Change
Migrates the group chat Members page (
DynamicReportParticipantsPage) off the legacySelectionListWithModal/TableListItemand onto the newTableframework introduced for workspace members in #91746. Because this page always renders inside the narrow RHP, it uses the Table's narrow (mobile) row layout, so it now looks like the workspace members table does on mobile: avatar + name + subtitle + caret, under a single "Members" header (no multi-column headers), matching the direction from@Expensify/designin the issue.This also resolves the originally reported bug (email tooltip only appearing on admin rows): the old inconsistency came from the
Adminbadge narrowing only admin rows' text column. Every row now shares an identical layout, so hover/tooltip behavior is consistent.New components (modeled on
WorkspaceMembersTable):src/components/Tables/ReportParticipantsTable/index.tsx— wrapsTable, defines themember/role(group chats only) /actionscolumns, sorts by name, and searches over name/email/login viatokenizedSearch.src/components/Tables/ReportParticipantsTable/ReportParticipantsTableRow.tsx— the narrow row (avatar + name + subtitle +ArrowRightcaret).Decisions applied (confirmed by
mkhutornyiin the issue):Admin • emailin the subtitle); non-admins show just their email, matching production. Members do not show a "Member" label.STANDARD_LIST_ITEM_LIMITor more active members, preserving the previous gating.Intentional behavior changes worth a reviewer's attention:
ROOM_MEMBERS_USER_SEARCH_PHRASE) is dropped; the Table owns search state (same as the workspace members table).Verification performed by MelvinBot (static checks only):
prettier,lint-changed, fulltsctypecheck (exit 0),tsgo(0 errors), and React Compiler compliance check (all three files COMPILED). No automated UI test exists for this page; visual/interaction QA is still needed (see Tests/QA).Fixed Issues
$ #93322
PROPOSAL: #93322 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
Admin • emailfor admins /emailfor non-admins + caret), matching the workspace members table on mobile.Offline tests
// TODO: The human co-author must fill out the offline tests.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screenshots/Videosundefined