fix(swap): mobile menu bar hide broken buttons (#6608) #6609
Conversation
WalkthroughReplaces JavaScript-based media checks with CSS media-query helpers by introducing Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@crutch12 is attempting to deploy a commit to the cow-dev Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/cowswap-frontend/src/modules/application/containers/AppMenu/styled.ts (1)
1-11: Clarify that thisisMobileQueryuses a different breakpoint than the MenuBar oneHere
isMobileQueryisMedia.upToMedium, whilelibs/ui’sMenuBar/styled.tsdefines its ownisMobileQuery = Media.upToLarge. Using the same name for different ranges can be confusing when reading code across modules.Consider either documenting this difference inline or giving this helper a more specific name (e.g.
isMobileMenuQueryor similar) to reflect that AppMenu’s notion of “mobile” is narrower than MenuBar’s.Before renaming or documenting, please verify that AppMenu is intentionally using the up‑to‑medium breakpoint and that this divergence from MenuBar’s up‑to‑large threshold matches the desired UX across devices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/cowswap-frontend/src/modules/application/containers/AppMenu/index.tsx(4 hunks)apps/cowswap-frontend/src/modules/application/containers/AppMenu/styled.ts(1 hunks)libs/ui/src/pure/MenuBar/index.tsx(3 hunks)libs/ui/src/pure/MenuBar/styled.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-10T20:28:16.565Z
Learnt from: fairlighteth
Repo: cowprotocol/cowswap PR: 6347
File: apps/cowswap-frontend/src/modules/trade/pure/TradeConfirmation/index.tsx:49-49
Timestamp: 2025-10-10T20:28:16.565Z
Learning: In apps/cowswap-frontend/src/modules/trade, TradeConfirmation follows a two-layer architecture: TradeConfirmationView (pure/stateless) in pure/TradeConfirmation/index.tsx renders the UI, while TradeConfirmation (container) in containers/TradeConfirmation/index.tsx wraps it to freeze props during pending trades (via useStableTradeConfirmationProps), wire in signing state (useSigningStep), and inject trade confirmation state (useTradeConfirmState). Consuming modules should import the container TradeConfirmation from 'modules/trade' to preserve this stateful behavior.
<!-- [add_learning]
When reviewing component refactoring in apps/cowswap-frontend/src/modules/trade, recognize the pattern where a pure view component (e.g., TradeConfirmationView) is separated from a stateful container (e.g., TradeConfirmation) that wraps it. The container adds runtime state management (prop stabilization, signing state, etc.) while the view remains testable and composable. Do not flag usages that import th...
Applied to files:
apps/cowswap-frontend/src/modules/application/containers/AppMenu/index.tsx
📚 Learning: 2025-09-11T08:25:51.460Z
Learnt from: alfetopito
Repo: cowprotocol/cowswap PR: 6234
File: libs/tokens/src/index.ts:1-4
Timestamp: 2025-09-11T08:25:51.460Z
Learning: In the cowprotocol/cowswap project, there is currently no SSR (Server-Side Rendering) support, so localStorage access at module import time does not cause SSR-related issues.
Applied to files:
apps/cowswap-frontend/src/modules/application/containers/AppMenu/index.tsx
🧬 Code graph analysis (2)
apps/cowswap-frontend/src/modules/application/containers/AppMenu/styled.ts (2)
libs/ui/src/pure/MenuBar/styled.ts (2)
isMobileQuery(773-773)HideMobile(775-779)libs/ui/src/consts.ts (1)
Media(23-43)
libs/ui/src/pure/MenuBar/index.tsx (2)
apps/cowswap-frontend/src/legacy/hooks/useMediaQuery.ts (1)
useMediaQuery(7-24)libs/ui/src/pure/MenuBar/styled.ts (3)
isMobileQuery(773-773)HideMobile(775-779)NavItems(166-234)
🔇 Additional comments (3)
libs/ui/src/pure/MenuBar/styled.ts (1)
3-3: SharedisMobileQuery+HideMobilehelper is a clean way to centralize the breakpointAliasing
Media.upToLargeasisMobileQueryand reusing it both in CSS (HideMobile) and via hooks inMenuBarkeeps the “mobile” breakpoint in one place and solves the initial flash without changing the breakpoint semantics.Also applies to: 773-779
apps/cowswap-frontend/src/modules/application/containers/AppMenu/index.tsx (1)
6-6: UsingisMobileQuery+HideMobilehere correctly removes the initial desktop flash for header childrenWiring
isMobilethroughuseMediaQuery(isMobileQuery(false))and wrappingchildrenin<HideMobile>for thepersistentAdditionalContentslot means:
- On mobile, the desktop header content is visually hidden immediately via CSS, even while
isMobileis stillfalseon the first render.- After the media query hook updates,
persistentAdditionalContentbecomesnull, so desktop-only content stays out of the mobile layout.This looks consistent with the intended behavior of that slot.
Please double‑check that
isMobileQuery(false)indeed returns a raw media query string (without the@mediaprefix) matching whatuseMediaQueryfrom@cowprotocol/common-hooksexpects, and that the chosen up‑to‑medium breakpoint aligns with the product definition of “mobile” for AppMenu.Also applies to: 22-23, 46-47, 113-114
libs/ui/src/pure/MenuBar/index.tsx (1)
51-53: ReusingisMobileQueryand wrappingNavItemsinHideMobilecleanly fixes the initial desktop flickerSwitching
isMobiletouseMediaQuery(isMobileQuery(false))keeps the breakpoint aligned with the styled helper, and wrapping the desktopNavItemsblock in<HideMobile>ensures that on mobile widths the desktop menu is CSS‑hidden immediately while the hook still reportsfalseon the first render. OnceisMobileflips totrue, the block isn’t rendered at all, so runtime behavior remains consistent with the prior logic without the flash.This is a solid approach for synchronizing JS and CSS behavior around the same breakpoint.
Please confirm that:
isMobileQuery(true)produces the correct@mediawrapper for styled‑components, andisMobileQuery(false)yields the raw media condition string thatuseMediaQueryexpects,so both CSS and JS stay in lockstep with the intended “mobile” breakpoint.
Also applies to: 938-940, 1004-1019
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| } | ||
| ` | ||
|
|
||
| export const isMobileQuery = Media.upToLarge |
There was a problem hiding this comment.
Nice context clarification!
apps/cowswap-frontend/src/modules/application/containers/AppMenu/styled.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/ui/src/pure/MenuBar/styled.ts (1)
776-780: LGTM! CSS-based hiding solves the initial render flash issue.The
HideMobilecomponent correctly usesisMobileQuery(true)to applydisplay: nonevia CSS media query. This ensures desktop-only elements are hidden immediately on mobile without waiting for JavaScript hydration, which directly addresses the initial render flash described in the PR objectives.Consider adding a JSDoc comment to document the purpose:
+/** + * Hides child elements on mobile viewports using CSS media queries. + * Used to prevent desktop-only UI from flashing on initial mobile render. + */ export const HideMobile = styled.div` ${isMobileQuery(true)} { display: none; } `
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/ui/src/pure/MenuBar/styled.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
libs/ui/src/pure/MenuBar/styled.ts (2)
apps/cowswap-frontend/src/modules/application/containers/AppMenu/styled.ts (2)
isMobileQuery(5-5)HideMobile(7-11)libs/ui/src/consts.ts (1)
Media(23-43)
⏰ 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). (2)
- GitHub Check: Setup
- GitHub Check: Cypress
🔇 Additional comments (2)
libs/ui/src/pure/MenuBar/styled.ts (2)
3-3: LGTM! Media import is necessary for the new exports.The addition of
Mediato the imports is correct and required for theisMobileQueryconstant defined below.
774-774: Verify the breakpoint consistency between MenuBar and AppMenu.The implementation correctly aliases
Media.upToLargefor MenuBar's mobile query. However,AppMenu/styled.tsdefines its ownisMobileQueryusingMedia.upToMedium, which is a different breakpoint.This means:
- MenuBar switches to mobile mode at the "Large" breakpoint
- AppMenu switches to mobile mode at the "Medium" breakpoint (earlier)
Please confirm this difference is intentional. If both components should transition to mobile mode at the same screen size, consider using a shared breakpoint constant.
elena-zh
left a comment
elena-zh
left a comment
There was a problem hiding this comment.
From the UI perspective changes LGTM
3 participants
Summary
Fixes #6608
Hide MenuBar buttons on mobile using css (media query)
Initial render screenshot:
cow.fi:
To Test
On Mobile:
Open /limit
See initial site rendering, or reload page
Should see no desktop buttons in MenuBar
Background
useMediaQueryusage). by defaultisMobile = false, thus mobile user sees desktop buttonsisMobilequery intoisMobileQueryto share this logic between conditional rendering (isMobile = ...) and media query displaying (const HideMobile = ...)Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.