fix(sidebar): keep language and theme controls visible in mobile drawer (fixes #3690) #3728
Conversation
|
@SamP231004 is attempting to deploy a commit to the umami-software Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Adds a new MobileLanguageButton component to make language and theme controls accessible in the mobile drawer, addressing issue #3690 where these controls were hidden in mobile view.
Key changes:
- Created
MobileLanguageButtoncomponent with language selector popover and theme toggle - Integrated component into
MobileNavto render within the mobile drawer - Note:
BarChart.tsxchanges are from a previous commit (d1be7bc) and not part of this PR
Critical issue identified:
The component uses position: fixed which positions elements relative to the viewport, not the modal container. This will place the button bar at the bottom of the entire screen rather than within the mobile drawer, potentially overlapping page content when the drawer is open. The desktop SideNav component uses normal flow positioning within a SidebarSection, which should be the model for the mobile implementation.
Confidence Score: 2/5
- This PR has a critical positioning bug that will cause incorrect rendering on mobile devices
- The use of
position: fixedin MobileLanguageButton will position the controls relative to the viewport instead of within the modal drawer, causing them to appear at the bottom of the screen rather than at the bottom of the drawer. This fundamentally breaks the intended functionality described in the PR. - src/components/input/MobileLanguageButton.tsx requires immediate attention - the fixed positioning approach needs to be replaced with normal flow positioning like the desktop SideNav implementation
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| src/components/input/MobileLanguageButton.tsx | 2/5 | New component uses position: fixed which positions relative to viewport instead of modal container, likely causing the buttons to appear outside the drawer. Multiple styling issues with positioning and layout. |
| src/app/(main)/MobileNav.tsx | 4/5 | Simple addition of MobileLanguageButton component to mobile drawer. Clean integration but effectiveness depends on the positioning issues in the new component. |
Sequence Diagram
sequenceDiagram
participant User
participant MobileNav
participant MobileMenuButton
participant Modal
participant MobileLanguageButton
participant LanguagePopover
participant ThemeButton
User->>MobileNav: Tap menu button
MobileNav->>MobileMenuButton: Open mobile drawer
MobileMenuButton->>Modal: Render modal (variant="sheet")
Modal->>MobileNav: Render children
MobileNav->>MobileLanguageButton: Render fixed bottom bar
Note over MobileLanguageButton: Uses position:fixed<br/>relative to viewport
MobileLanguageButton->>LanguagePopover: Render globe button
MobileLanguageButton->>ThemeButton: Render theme toggle
User->>LanguagePopover: Tap globe button
LanguagePopover->>User: Show language list (placement="top")
User->>LanguagePopover: Select language
LanguagePopover->>MobileLanguageButton: saveLocale(value)
User->>ThemeButton: Tap theme button
ThemeButton->>User: Toggle theme
3 files reviewed, 4 comments
| position: 'fixed', | ||
| bottom: 0, | ||
| left: 0, | ||
| padding: 16, | ||
| backgroundColor: 'var(--background)', | ||
| borderTop: '1px solid var(--border)', | ||
| gap: 8, | ||
| justifyContent: 'center', | ||
| zIndex: 1000, | ||
| }} |
There was a problem hiding this comment.
logic: position: fixed positions relative to the viewport, not the modal/dialog container. This will place the button bar at the bottom of the entire screen rather than within the mobile drawer, potentially overlapping other page content when the drawer is open.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/input/MobileLanguageButton.tsx
Line: 18:27
Comment:
**logic:** `position: fixed` positions relative to the viewport, not the modal/dialog container. This will place the button bar at the bottom of the entire screen rather than within the mobile drawer, potentially overlapping other page content when the drawer is open.
How can I resolve this? If you propose a fix, please make it concise.| style={{ | ||
| position: 'fixed', | ||
| bottom: 0, | ||
| left: 0, |
There was a problem hiding this comment.
style: Missing right: 0 or width: 100% means the bar won't stretch full width across the screen.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/input/MobileLanguageButton.tsx
Line: 20:20
Comment:
**style:** Missing `right: 0` or `width: 100%` means the bar won't stretch full width across the screen.
How can I resolve this? If you propose a fix, please make it concise.| left: '0 !important', | ||
| right: '0 !important', | ||
| marginBottom: 16, | ||
| position: 'fixed', | ||
| }} |
There was a problem hiding this comment.
style: Using !important and setting both left and right to 0 is unusual. The popover positioning may conflict with the fixed positioning of the parent Row, causing rendering issues or misalignment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/input/MobileLanguageButton.tsx
Line: 40:44
Comment:
**style:** Using `!important` and setting both `left` and `right` to 0 is unusual. The popover positioning may conflict with the fixed positioning of the parent Row, causing rendering issues or misalignment.
How can I resolve this? If you propose a fix, please make it concise.| position: 'fixed', | ||
| }} | ||
| > | ||
| <Dialog variant="menu" style={{ maxHeight: '60vh', overflowY: 'auto' }}> |
There was a problem hiding this comment.
style: Using maxHeight: '60vh' with overflowY: 'auto' inside a modal that's already constrained may cause scrolling issues if the language list is long and combined with the fixed bottom positioning.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/input/MobileLanguageButton.tsx
Line: 46:46
Comment:
**style:** Using `maxHeight: '60vh'` with `overflowY: 'auto'` inside a modal that's already constrained may cause scrolling issues if the language list is long and combined with the fixed bottom positioning.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
On mobile view, the sidebar (rendered as a dialog/sheet) hid the Language and Theme buttons.
To address this, a new
MobileLanguageButtoncomponent has been introduced to render these controls as a fixed bottom bar, ensuring they remain visible and usable across all screen sizes.Implementation Details
MobileLanguageButtoncomponent that:MenuTrigger+Popoverto display the language selector above the bar (avoiding clipping issues).MobileLanguageButtonon mobile viewports.Popoverusesplacement="top"and fixed positioning to remain fully visible.Why this fix
The original sidebar layout placed the language and theme buttons within the
SidebarSection, which became hidden or clipped when the sidebar was rendered as a modal on mobile (Dialog_sheet).This component-based approach keeps the buttons consistently visible, independent of the sidebar container’s rendering mode.
Screenshots
(Current look after fix)
Related Issue
Fixes #3690
Checklist