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

Fix long list output in Dropdown #535

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

natalyjazzviolin
Copy link
Contributor

@natalyjazzviolin natalyjazzviolin commented Jan 29, 2025

Closes #533

Why

Currently lists over 15+ items in the Dropdown component do not display correctly:
Screenshot 2025-01-29 at 4 55 54 PM

What

This adds overflow-y: auto to the Dropdown.Content component to allow for scrolling and sets min-height: 32px on the Dropdown.Item to make sure it does not get compressed to less than the expected height. I got the 32px from the computed height of other Dropdown.Items I looked at.

Screen.Recording.2025-01-29.at.4.54.24.PM.mov

Copy link

vercel bot commented Jan 29, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
click-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 4, 2025 1:47pm

@natalyjazzviolin natalyjazzviolin marked this pull request as ready for review January 29, 2025 22:02
@@ -12,6 +12,7 @@ export const Dropdown = (props: DropdownMenu.DropdownMenuProps) => (
const DropdownMenuItem = styled(GenericMenuItem)`
position: relative;
display: flex;
min-height: 32px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we adding min-height here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vineethasok I found that just adding overflow-y: auto on the parent container was not enough to maintain the expected height of the Dropdown.Item elements.

I think it might be because there is a max expected height of the parent container and the height of this child component is calculated based on that; the more children, the less their height to fit inside the parent

@@ -93,6 +94,7 @@ const DropdownMenuContent = styled(GenericMenuPanel)`
min-width: ${({ theme }) => theme.click.genericMenu.item.size.minWidth};
Copy link
Collaborator

Choose a reason for hiding this comment

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

@natalyjazzviolin Maybe we can also add maxHeight as a prop which can be provided by the user
@gjones Would love your thoughts

@danielclickh
Copy link

  1. Would it be possible to have a dark mode version of scroll to match the rest of the UI?
  2. I would probably have it a bit shorter, to prevent the UI to go too close to the border;

@natalyjazzviolin
Copy link
Contributor Author

  • Added a max height style, would love direction here if possible as I picked something that looked arbitrarily good.
  • Re: scrollbar styling - in Firefox it looks correct. In my Chrome instance it doesn't adhere to dark mode, but it's likely specific to my environment. I debugged this together with @hoorayimhelping and the scrollbar was dark-themed in his envionment
Screenshot 2025-01-31 at 2 10 27 PM

@@ -93,6 +94,10 @@ const DropdownMenuContent = styled(GenericMenuPanel)`
min-width: ${({ theme }) => theme.click.genericMenu.item.size.minWidth};
flex-direction: column;
z-index: 1;
overflow-y: scroll;
max-height: calc(
(var(--radix-${({ $type }) => $type}-content-available-height) - 100px)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, what does 100px represent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hoorayimhelping this subtracts 100px from the available height so the bottom of the dropdown doesn't get too close to the bottom of the screen per Daniel's request

@natalyjazzviolin natalyjazzviolin merged commit 9ea5194 into main Feb 4, 2025
6 checks passed
@natalyjazzviolin natalyjazzviolin deleted the nataly/fix-dropdown-overflow branch February 4, 2025 15:15
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.

Dropdown: UI breaks with long list of items
5 participants