-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
fixed long list items in dropdown were hidden, rename <Menu />
to <DropdownMenu />
#3206
Conversation
🦋 Changeset detectedLatest commit: 1216ef1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
); | ||
} | ||
|
||
export const Menu = createComponentGroup(DropdownMenu.Root, { |
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.
@thomasheyenbrock I renamed Menu to DropdownMenu because
- for me menu, listbox are confused names
- we follow radix convention
- even this file is name dropdown 😅
- major release is coming so we can do some adjustments :)
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.
Sounds good 👍
Since we're still on 0.x
, I'd even be fine with doing this in a minor. The expectations around @graphiql/react
always were that it's not yet stable.
[data-radix-menu-content] { | ||
.graphiql-dropdown-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.
to avoid conflicts with users’ maybe radix components
@@ -8,9 +8,11 @@ | |||
padding: var(--px-4); | |||
font-family: var(--font-family); | |||
color: hsl(var(--color-neutral)); | |||
max-height: calc(var(--radix-dropdown-menu-content-available-height) - 10px); |
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.
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.
Using all the available height seems too much for me, having the dropdown fill the whole height of the page looks weird. I'd propose to go with a smaller value here like 400px
.
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.
hardcoding 400px would provoke issues on small vh
, we should use css' min
function with it
fixes #3187 cc @jetsstarplus
Screen.Recording.2023-06-02.at.01.30.05.mov