Skip to content
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

Scrollable fixed dropdowns container minor refactor #9159

Conversation

ehconitin
Copy link
Contributor

No description provided.

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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 standardizes dropdown menu behavior and structure across multiple components in the Twenty frontend.

  • Changed withoutScrollWrapper prop to scrollable={false} in DropdownMenuItemsContainer components for more intuitive API
  • Removed unnecessary DropdownMenu wrapper components while preserving functionality in summary cards and select forms
  • Added DropdownMenuSeparator components between content sections for better visual hierarchy
  • Fixed save/action buttons to be non-scrollable in view pickers and relation selectors
  • Simplified dropdown menu structure by removing redundant fragment wrappers and organizing imports

20 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile

@@ -87,7 +87,7 @@ export const ObjectOptionsDropdownHiddenFieldsContent = () => {
closeDropdown();
}}
>
<DropdownMenuItemsContainer withoutScrollWrapper>
<DropdownMenuItemsContainer scrollable={false}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: scrollable={false} is only needed for single menu items that don't require scrolling. Consider removing this prop since it's redundant here - the container will never need to scroll with just one menu item.

@@ -144,7 +144,7 @@ export const MultiRecordSelect = ({
{dropdownPlacement?.includes('end') && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: duplicate code structure between end and start placement branches. Consider extracting into a shared function to reduce code duplication

@lucasbordeau lucasbordeau self-assigned this Dec 20, 2024
Copy link
Contributor

@lucasbordeau lucasbordeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work !

@lucasbordeau lucasbordeau enabled auto-merge (squash) December 20, 2024 09:40
@charlesBochet charlesBochet merged commit 54c4d64 into twentyhq:main Dec 20, 2024
19 of 20 checks passed
Copy link

Thanks @ehconitin for your contribution!
This marks your 76th PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

mdrazak2001 pushed a commit to mdrazak2001/twenty that referenced this pull request Dec 20, 2024
samyakpiya pushed a commit to samyakpiya/twenty that referenced this pull request Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants