feat: size table columns from their content - #97755
luacmartins merged 16 commits into
Conversation
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…erflow style Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@MelvinBot review |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Review — dynamic table column widthsOverall this is a clean, well-documented implementation that matches all three behaviors from #96510. The pure width algorithm is nicely factored out and unit-tested, and the native/web split correctly makes this a web-only, wide-layout-only enhancement (native
Two things worth confirming before this merges: 1. Behavior #3 (horizontal scroll) is effectively unreachable for the Members tableThe issue's third behavior — "if all columns are long and can't fit, allow horizontal scrolling" — could not be reproduced at any wide-layout viewport (tested 1026px → 1920px) even with two maximally long custom-field values. Instead the columns proportionally shrink and ellipsis-truncate. The reason is that a dynamic column's This isn't a code defect — the scroll path is implemented and unit-tested at the algorithm level — but it does mean the Members table never exercises behavior #3, and long values silently truncate. Worth confirming with reviewers whether truncation-instead-of-scroll is acceptable for this first adopter, or whether the Member column should carry a larger 2.
|
…dable width Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
@eVoloshchak 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 670eb12d84
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…cifies Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
🚧 dubielzyk-expensify has triggered a test Expensify/App build. You can view the workflow run here. |
|
@eVoloshchak can you please prioritize this review when you get a chance? |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…t measure text Signed-off-by: krishna2323 <belivethatkg@gmail.com>
|
I kinda think that is expected, that feels way nicer now since they are balanced. |
|
Tested and it's feeling pretty great! Agree that spend page is the real test, but this is super promising and already just a way better experience |
Is this expected? It seems like we can safely truncate Custom field 2 and Custom field 1 in this case Screen.Recording.2026-08-19.at.18.32.35.mov |
|
Curious what @shawnborton thinks. I don't know if we have a clear rule for when we start horizontal scroll. I think custom fields 1/2 isn't crucial info so I can see truncating it, but I also don't mind the scroll. Role feels important though but the width is a bit small when it actually stops showing anyways |
|
Hmm yeah that's a tricky one. I would assume we have some kind of min-width on the cells (at least ones that have long strings) and the summation of the minWidths across multiple cells in a horizontal row is what would trigger the horizontal scrolling? Sounds like a mouthful. Not sure how we implement it, but I guess I also feel like the video above is okay? |
Yeah I think that makes. Totally fine with the video as well and we can adjust if we hear complaints, but I ultimately also think the system is probably working as intended |
|
Its working as intended. Each column has a minimum width, and the table only scrolls when the sum of those minimums doesn't fit. The four states are:
For the current defaults:
The important bit is Role is different since it's |
|
Cool. @eVoloshchak please continue your review |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppscreen-20260821-164244-1787323354302.mp4Android: mWeb ChromeScreen.Recording.2026-08-21.at.16.35.30.moviOS: HybridAppScreen.Recording.2026-08-21.at.16.33.14.moviOS: mWeb SafariScreen.Recording.2026-08-21.at.16.30.43.movMacOS: Chrome / SafariScreen.Recording.2026-08-21.at.16.27.05.mov |
|
@eVoloshchak looks like some checkboxes are missing, can you finish the checklist please? |
| * Upper bound on cached measurements. Text measurement is only used to size layouts, so a coarse cap is enough to keep | ||
| * the cache from growing without bound on long-lived sessions. | ||
| */ | ||
| const MAX_CACHE_SIZE = 5000; |
There was a problem hiding this comment.
nab, did we want to lower this? I think we can reduce it a small bit
There was a problem hiding this comment.
I agree that we can lower this. Let's do it in a follow up though
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 luacmartins has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.4.59-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site articles under Why: This PR is a purely visual/rendering enhancement — it sizes table columns dynamically from their content (introducing a The closest article, Managing-Workspace-Members.md, describes members and custom fields as data/import concepts (e.g. mapping Custom field 1/Custom field 2 during import). None of it describes column widths or table layout, and this PR doesn't change any of that behavior — so nothing in it needs updating. If you intended a specific user-facing behavior change that should be documented, let me know and I'll draft the docs PR. |
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.4.59-5 🚀
Bundle Size Analysis (Sentry): |



Explanation of Change
Fixed Issues
$ #96510
PROPOSAL:
Tests
Setup
Create a Control workspace (custom field columns only render on Control).
Invite ~5 members, including:
Open staging.new.expensify.com in a maximized desktop browser.
Go to Workspace settings → Members.
Test Steps
With no custom fields set, verify:
Set Custom field 1 to short values (e.g.
A1,B2,C3) for a few members.Change one Custom field 1 value to a long string (e.g.
Engineering-Department-Payroll-Reference-2026-Q3).Verify:
Set long values for Custom field 2 on several members.
Narrow the browser while staying in desktop layout.
Verify:
Search members, clear the search, apply a Role filter, and sort by Member.
Select individual members and then use Select all.
Resize the browser between desktop and tablet widths.
Switch to tablet/mobile width.
On iOS and Android (portrait & landscape), open the same workspace.
main.Verify these tables are unchanged:
Offline tests
Go offline, invite a member, and verify:
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
ios_mWeb.mp4
MacOS: Chrome / Safari
Monosnap.screencast.2026-08-17.10-52-16.mp4