Sort join-workspaces by member count and add Show more#86884
Sort join-workspaces by member count and add Show more#86884
Conversation
Sort available workspaces by employee count (descending) so the largest workspaces appear first. When more than 5 workspaces are available, show only the first 5 with a "Show more" button to reveal the rest. Co-authored-by: Mykhailo <mkhutornyi@users.noreply.github.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
| <Button | ||
| success={false} | ||
| large | ||
| text={translate('common.skip')} |
There was a problem hiding this comment.
@MelvinBot The button copy should be "Skip for now"
There was a problem hiding this comment.
Updated the button copy to "Skip for now" — added onboarding.skipForNow translation key across all language files.
| <Button | ||
| large | ||
| text={translate('common.showMore')} | ||
| onPress={() => setShowAll(true)} | ||
| style={[styles.mt5]} | ||
| /> |
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed — changed "Show more" to use the link style (matching the existing pattern in TransactionGroupListExpanded) and moved it from footerContent to listFooterContent so it renders right after the list items inside the scrollable area.
…inWorkspaces
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>
# Conflicts:
# src/pages/OnboardingWorkspaces/BaseOnboardingWorkspaces.tsx
|
Merged |
… now Co-authored-by: Mykhailo <mkhutornyi@users.noreply.github.com>
🦜 Polyglot Parrot! 🦜Squawk! Looks like you added some shiny new English strings. Allow me to parrot them back to you in other tongues: The diff is too large to include in this comment (62KB), so I've created a gist for you: 📋 View the translation diff here 📋 Note You can apply these changes to your branch by copying the patch to your clipboard, then running |
| hasMoreThanFive && !showAll ? ( | ||
| <View style={[wrapperPadding]}> | ||
| <Button | ||
| text={translate('common.showMore')} |
There was a problem hiding this comment.
@MelvinBot this button should be left aligned. And there should be dropdown arrow icon next to the text.
There was a problem hiding this comment.
Done — left-aligned the Show more button (via alignItemsStart on the wrapper) and added a down arrow icon next to the text (via iconRight).
|
The only failing check is PR Reviewer Checklist — no reviewer has posted their checklist yet. This is not a code issue and doesn't require a fix. All code checks (tests, ESLint, typecheck, prettier, builds, spellcheck, etc.) are passing. |
Co-authored-by: Mykhailo <mkhutornyi@users.noreply.github.com>
|
The only failing check is PR Reviewer Checklist — no reviewer has posted their checklist comment yet. This is not a code issue and doesn't require a fix. All code checks (ESLint, prettier, spellcheck, builds) are passing. Tests and typecheck are still running. |
|
The failing Failing test:
Why it's unrelated:
The other failing check ( Recommendation: Re-run the failed jobs. This should pass on retry. |
| innerStyles={[styles.ph3]} | ||
| textStyles={[styles.fontSizeNormal]} |
There was a problem hiding this comment.
| innerStyles={[styles.ph3]} | |
| textStyles={[styles.fontSizeNormal]} | |
| shouldShowRightIcon | |
| innerStyles={styles.pl0} |
There's still left padding even after applying styles.pl0. It's overridden by padding-left: 16px;. Can you investigate why?
There was a problem hiding this comment.
The padding-left: 16px comes from buttonMedium style (src/styles/index.ts:781) which sets paddingHorizontal: 16. React Native Web preprocesses paddingHorizontal into paddingInline, while pl0 uses paddingLeft — styleq sees these as different property keys and applies both CSS classes to the DOM element. The cascade usually resolves correctly (RNW puts paddingLeft in group 3 vs paddingInline in group 2.1), but it's fragile since it depends on internal stylesheet ordering.
Fix: Use innerStyles={styles.ph0} instead of styles.pl0. This overrides with paddingHorizontal: 0, matching the same shorthand level as buttonMedium, so styleq correctly deduplicates them and only one class is applied.
There was a problem hiding this comment.
| innerStyles={[styles.ph3]} | |
| textStyles={[styles.fontSizeNormal]} | |
| shouldShowRightIcon | |
| innerStyles={styles.ph0} |
Also, there should be iconRightHoverFill and iconRightFill to match text style.
There was a problem hiding this comment.
Done — added shouldShowRightIcon, changed innerStyles to styles.ph0, and added iconRightFill={theme.link} and iconRightHoverFill={theme.linkHover} so the arrow icon matches the link text color.
…icon fill colors
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>
|
🤖 Code Review Two issues to address: 1. The sort + map runs on every render. Wrap it in 2. Magic number The limit Minor / non-blocking
|
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppios.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
…inWorkspaces Co-authored-by: Mykhailo <mkhutornyi@users.noreply.github.com>
…m/Expensify/App into claude-sortAndLimitJoinWorkspaces Co-authored-by: Mykhailo <mkhutornyi@users.noreply.github.com>
|
The failing test ( |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
…and move useState to hook group Co-authored-by: Mykhailo <mkhutornyi@users.noreply.github.com>
|
The only failing checks on this PR are PR Reviewer Checklist, which is an expected checklist reminder — not a real CI failure. All other checks (ESLint, Prettier, typecheck, tests, builds, CLA, etc.) are either passing or still running. There are no actionable CI failures to fix at this time. If any of the currently pending checks (tests, typecheck, perf-tests, etc.) fail once they complete, feel free to tag me again and I'll investigate. |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
…s' into claude-sortAndLimitJoinWorkspaces Co-authored-by: Mykhailo <mkhutornyi@users.noreply.github.com>
|
Addressed the review feedback from this comment:
|
|
@MelvinBot mark as ready for review |
|
Done — marked this PR as ready for review. |
melvin-bot
bot
requested review from
heyjennahay and
mkhutornyi
and removed request for
a team
|
@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] |
|
@Expensify/design please review web.mov(Ignore that only 2 items show. It's because I haven't found test domain which has more than 5 workspaces. It should be 5 items in reality) |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #86192 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
That's looking pretty good to me 👍 |
|
Looking good to me too 👍 |
|
@heyjennahay all yours! |
6 participants
Explanation of Change
Updates the
/join-workspacesonboarding modal to:These changes improve the onboarding experience by surfacing the most popular workspaces first and reducing visual clutter when many workspaces are available.
Fixed Issues
$ #86192
PROPOSAL: #86192 (comment)
Tests
/join-workspacesonboarding stepOffline tests
/join-workspacesscreen with workspaces loadedQA Steps
Same as Tests above.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari