-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIX] slash-menu-height-fix #8327
[FIX] slash-menu-height-fix #8327
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR modifies the DropdownMenu component to fix excessive vertical spacing in the slash menu by introducing height control through an isCustomSlashMenu
prop.
- Added
isCustomSlashMenu
boolean prop toDropdownMenu
in/packages/twenty-front/src/modules/ui/layout/dropdown/components/DropdownMenu.tsx
to toggle betweenfit-content
and100%
height - Implemented prop usage in
/packages/twenty-front/src/modules/ui/input/editor/components/CustomSlashMenu.tsx
by settingisCustomSlashMenu={true}
- Consider refactoring to use a more generic height control prop rather than slash-menu specific implementation for better maintainability
2 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -24,7 +24,7 @@ const StyledSlashMenu = styled.div` | |||
export const CustomSlashMenu = (props: CustomSlashMenuProps) => { | |||
return ( | |||
<StyledSlashMenu> | |||
<DropdownMenu style={{ zIndex: 2001 }}> | |||
<DropdownMenu style={{ zIndex: 2001 }} isCustomSlashMenu={true}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider adding a maxHeight prop to handle cases where the content might exceed the viewport height
height: ${({ isCustomSlashMenu }) => | ||
isCustomSlashMenu ? 'fit-content' : '100%'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider using a more generic prop name like fitContent
instead of isCustomSlashMenu
to make this feature reusable for other dropdown cases
box-shadow: ${({ theme }) => theme.boxShadow.strong}; | ||
|
||
display: flex; | ||
|
||
height: 100%; | ||
height: ${({ isCustomSlashMenu }) => | ||
isCustomSlashMenu ? 'fit-content' : '100%'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Hitarthsheth07 fit-content was not working everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does work everywhere, should I change it to fit-content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it works everywhere yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ob it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Thanks @Hitarthsheth07 for your contribution! |
FIX #8326
I've used the
height: fit-content
property just for customSlashMenu but I think it will work for all dropdown menu's.I tested it for a few and works fine, but not sure for edge cases.
Let me know if the height should be changed to
fir-content