Fix missing menu border padding #11782
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
Fixed dropdown menu padding regression by adjusting container spacing and scroll wrapper styling.
- Modified padding in
StyledDropdownMenuItemsExternalContainertotheme.spacing(1, 1, 0, 1)to remove bottom padding - Added
margin-bottom: theme.spacing(1)toStyledScrollWrapperfor consistent spacing - Replaced basic ScrollWrapper with StyledScrollWrapper in scrollable case for uniform styling
💡 (3/5) Reply to the bot's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
Hey @abdulrahmancodes thanks for your contribution
Unfortunately this brings the following regression due to StyledDropdownMenuItemsExternalContainer bottom-margin removal
Ah, got it. Didn’t realize that was affecting layout elsewhere. I’ll investigate and make sure it is fixed. |
There was a problem hiding this comment.
Decided to refactor the component globally as it was kinda hard to get through in the first place
Not sure about what container should handle the spacing, fixed and implemented a solution following @abdulrahmancodes initial idea
@lucasbordeau same here if you could give this an review please 🙏
packages/twenty-front/src/modules/ui/layout/dropdown/components/DropdownMenuItemsContainer.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/dropdown/components/DropdownMenuItemsContainer.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/dropdown/components/DropdownMenuItemsContainer.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/dropdown/components/DropdownMenuItemsContainer.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/dropdown/components/DropdownMenuItemsContainer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Created a new abstraction to handle ScrollWrapper dynamic display in order to simplify initial component and restored padding attributes usage only
I think we should mainly good to go with this, @lucasbordeau could you please have a look ?
|
Hey @prastoin , just wanted to check - is the padding after the scrollbar okay? Screen.Recording.2025-04-30.at.2.53.53.PM.mov |
Ah yes my bad you're right, about to fix |
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:55195 This environment will automatically shut down when the PR is closed or after 5 hours. |
| import { PropsWithChildren, useId } from 'react'; | ||
|
|
||
| const StyledScrollWrapper = styled(ScrollWrapper)` | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
Open question: Not sure why this is required here but it is, not getting globally applied from the index.css
There was a problem hiding this comment.
Here is a repro, first part of the video with StyledPaddingBlockContainer second commented StyledPaddingBlockContainer
were you talking about that @lucasbordeau ?
Screen.Recording.2025-04-30.at.16.46.18.mov
|
Main : Enregistrement.de.l.ecran.2025-04-30.a.16.59.34.movThis branch : Enregistrement.de.l.ecran.2025-04-30.a.17.00.00.mov |
Demo
Screen.Recording.2025-04-29.at.10.46.38.AM.mov
Closes #11766