fix: Wrong spacing between filters and search bar in Marketplace #38617
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughTwo UI components are updated to support responsive layouts. FilterByText adds mobile-based spacing adjustments using breakpoint detection, while AppsFilters applies conditional layout properties to form components based on desktop versus non-desktop screen sizes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/marketplace/AppsPage/AppsFilters.tsx`:
- Around line 64-73: The bottom margin on mobile is being applied to every child
inside FilterByText (including the last RadioDropDown for the sort filter), so
remove or override the mobile margin for the final filter element; locate the
FilterByText usage and either (A) apply a CSS rule targeting the last child
(e.g., :last-child) to remove mbe:8 on mobile, or (B) pass a prop/flag to the
last rendered control (the RadioDropDown using sortFilterStructure and
sortFilterOnSelected) or CategoryDropDown to suppress the mobile margin via
filterLayoutProps for that last item so the TagList has no extra spacing. Ensure
you reference the last RadioDropDown (sort filter) and adjust styling logic
consistently for isPrivateAppsPage conditional branches.
🧹 Nitpick comments (2)
apps/meteor/client/components/FilterByText.tsx (1)
18-22: Breakpoint detection approach is inconsistent with AppsFilters.tsx.This file uses
!breakpoints.includes('lg')whileAppsFilters.tsxuses!['xs', 'sm', 'md'].includes(breakpoints[breakpoints.length - 1])to determine the same mobile/desktop threshold. Both yield the same result, but using two different idioms for the same check across related files is a maintenance hazard. Consider aligning on one approach.apps/meteor/client/views/marketplace/AppsPage/AppsFilters.tsx (1)
58-60:fixFiltersSizecan be inlined for clarity.
fixFiltersSizeis only consumed inside the desktop branch offilterLayoutProps. You could simplify by inlining it directly:const filterLayoutProps = isDesktop ? { flexGrow: 1, maxWidth: 'x200', minWidth: 'x200' } : { w: 'full', mbe: 8 };Also note that with identical
minWidthandmaxWidth,flexGrow: 1has no observable effect — the element is effectively fixed at 200px.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/client/components/FilterByText.tsxapps/meteor/client/views/marketplace/AppsPage/AppsFilters.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/components/FilterByText.tsxapps/meteor/client/views/marketplace/AppsPage/AppsFilters.tsx
🧠 Learnings (3)
📚 Learning: 2025-11-17T15:07:13.273Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37398
File: packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx:357-363
Timestamp: 2025-11-17T15:07:13.273Z
Learning: In packages/fuselage-ui-kit/src/surfaces/FuselageSurfaceRenderer.tsx, IconElement is a presentational, non-actionable element that does not require wrapping in AppIdProvider, similar to plain_text and mrkdwn renderers. Only actionable elements (those with actions, actionId, or interactive behavior) should be wrapped in AppIdProvider.
Applied to files:
apps/meteor/client/components/FilterByText.tsx
📚 Learning: 2026-01-19T18:08:13.423Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38169
File: packages/ui-client/src/hooks/useGoToDirectMessage.ts:20-27
Timestamp: 2026-01-19T18:08:13.423Z
Learning: In React hooks, including custom hooks like useUserSubscriptionByName, must always be called unconditionally and in the same order on every render, per the Rules of Hooks. Passing empty strings or fallback values to hooks is the correct approach when dealing with potentially undefined values, rather than conditionally calling the hook based on whether the value exists.
Applied to files:
apps/meteor/client/components/FilterByText.tsx
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/client/views/marketplace/AppsPage/AppsFilters.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (1)
apps/meteor/client/components/FilterByText.tsx (1)
28-41: Spacing logic for the mobile layout looks correct.The
mbeon the search bar wrapper (line 30) creates vertical separation when filters wrap below it on narrow viewports, and the parentflexWrap='wrap'on line 29 enables the stacking. The children filters receive their ownmbeviafilterLayoutPropsinAppsFilters.tsx, so the overall vertical rhythm should be consistent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| <FilterByText value={text} onChange={(event) => setText(event.target.value)} placeholder={appsSearchPlaceholders[context]}> | ||
| {!isPrivateAppsPage && ( | ||
| <RadioDropDown group={freePaidFilterStructure} onSelected={freePaidFilterOnSelected} flexGrow={1} {...fixFiltersSize} /> | ||
| <RadioDropDown group={freePaidFilterStructure} onSelected={freePaidFilterOnSelected} {...filterLayoutProps} /> | ||
| )} | ||
| <RadioDropDown group={statusFilterStructure} onSelected={statusFilterOnSelected} flexGrow={1} {...fixFiltersSize} /> | ||
| <RadioDropDown group={statusFilterStructure} onSelected={statusFilterOnSelected} {...filterLayoutProps} /> | ||
| {!isPrivateAppsPage && ( | ||
| <CategoryDropDown categories={categories} selectedCategories={selectedCategories} onSelected={onSelected} flexGrow={1} /> | ||
| <CategoryDropDown categories={categories} selectedCategories={selectedCategories} onSelected={onSelected} {...filterLayoutProps} /> | ||
| )} | ||
| <RadioDropDown group={sortFilterStructure} onSelected={sortFilterOnSelected} flexGrow={1} {...fixFiltersSize} /> | ||
| <RadioDropDown group={sortFilterStructure} onSelected={sortFilterOnSelected} {...filterLayoutProps} /> | ||
| </FilterByText> |
There was a problem hiding this comment.
Last filter in mobile view gets unnecessary bottom margin.
All filters receive mbe: 8 on mobile, including the last RadioDropDown (sort filter, line 72). This adds extra bottom spacing before the TagList. Consider either removing the margin from the last child (e.g., via CSS :last-child or conditional logic) or verifying the visual result is acceptable.
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/marketplace/AppsPage/AppsFilters.tsx` around lines
64 - 73, The bottom margin on mobile is being applied to every child inside
FilterByText (including the last RadioDropDown for the sort filter), so remove
or override the mobile margin for the final filter element; locate the
FilterByText usage and either (A) apply a CSS rule targeting the last child
(e.g., :last-child) to remove mbe:8 on mobile, or (B) pass a prop/flag to the
last rendered control (the RadioDropDown using sortFilterStructure and
sortFilterOnSelected) or CategoryDropDown to suppress the mobile margin via
filterLayoutProps for that last item so the TagList has no extra spacing. Ensure
you reference the last RadioDropDown (sort filter) and adjust styling logic
consistently for isPrivateAppsPage conditional branches.
1 participant
Proposed changes (including videos or screenshots)
The changes made below fixes the no gap between filters in Marketplace issue. As of now the changes set the width of the filters and search bar to be full if the screen width is below 1024px. The design could be changed based on the maintainers preference.
#38613
Before Changes:
After Changes:
Steps to test or reproduce
Summary by CodeRabbit