fix: TimeZone dropdown clips through screen in Mobile viewport #7935
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR addresses the timezone dropdown clipping and transparency issues in mobile viewports by introducing responsive width adjustments to the Dropdown and DropdownMenu components.
- Added 'mobileDropdownWidth' prop to Dropdown.tsx for viewport-specific width control
- Implemented MOBILE_VIEWPORT constant in DropdownMenu.tsx to determine appropriate width
- Modified DropdownMenu styling to reduce transparency and improve visibility
- Adjusted dropdown width calculation in Dropdown.tsx to consider mobile viewport
- Ensured full-width rendering for desktop filters dropdown as per issue requirements
2 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| const dropdownWidth = | ||
| mobileDropdownWidth && window.innerWidth <= MOBILE_VIEWPORT | ||
| ? mobileDropdownWidth | ||
| : dropdownMenuWidth; |
There was a problem hiding this comment.
logic: This logic might cause issues if window is undefined in SSR environments
| useDropdown(dropdownId); | ||
|
|
||
| const offsetMiddlewares = []; | ||
| const dropdownWidth = | ||
| mobileDropdownWidth && window.innerWidth <= MOBILE_VIEWPORT |
There was a problem hiding this comment.
style: Consider using a hook like useMediaQuery for responsive behavior instead of directly checking window.innerWidth
| @@ -1,9 +1,10 @@ | |||
| import styled from '@emotion/styled'; | |||
|
|
|||
| import { MOBILE_VIEWPORT } from 'twenty-ui'; | |||
There was a problem hiding this comment.
style: Consider using a more specific import for MOBILE_VIEWPORT
| width: ${({ width = 160 }) => | ||
| typeof width === 'number' ? `${width}px` : width}; | ||
| width: ${({ width = 160, mobileDropdownWidth }) => { | ||
| const isMobileViewport = MOBILE_VIEWPORT; |
There was a problem hiding this comment.
logic: MOBILE_VIEWPORT is likely a boolean, not a function. Remove unnecessary const assignment
| typeof width === 'number' ? `${width}px` : width}; | ||
| width: ${({ width = 160, mobileDropdownWidth }) => { | ||
| const isMobileViewport = MOBILE_VIEWPORT; | ||
| if (Boolean(isMobileViewport) && Boolean(mobileDropdownWidth)) { |
There was a problem hiding this comment.
style: Unnecessary Boolean() wrapping, MOBILE_VIEWPORT is already a boolean
| width: ${({ width = 160, mobileDropdownWidth }) => { | ||
| const isMobileViewport = MOBILE_VIEWPORT; | ||
| if (Boolean(isMobileViewport) && Boolean(mobileDropdownWidth)) { | ||
| return typeof mobileDropdownWidth === 'number' | ||
| ? `${mobileDropdownWidth}px` | ||
| : mobileDropdownWidth; | ||
| } | ||
| return typeof width === 'number' ? `${width}px` : width; | ||
| }}; |
There was a problem hiding this comment.
style: Consider extracting this logic into a separate function for better readability
|
|
|
Fixed with new property |
Description