-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[base-ui][Menu] Use Popup instead of Popper #40731
Conversation
Netlify deploy preview@material-ui/unstyled: parsed: -0.03% 😍, gzip: -0.21% 😍 Bundle size reportDetails of bundle changes (Toolpad) |
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.
Overall looks good 😊
I like that we're battle testing the Transitions and Popup API right away and simplifying them ✨
Just had some questions
@@ -71,7 +76,7 @@ const Menu = React.forwardRef((props, ref) => { | |||
return { | |||
...resolvedSlotProps, | |||
className: clsx( | |||
'text-sm box-border font-sans p-1.5 my-3 mx-0 rounded-xl overflow-auto outline-0 bg-white dark:bg-slate-900 border border-solid border-slate-200 dark:border-slate-700 text-slate-900 dark:text-slate-300 min-w-listbox shadow-md dark:shadow-slate-900', | |||
'text-sm box-border font-sans p-1.5 my-3 mx-0 rounded-xl overflow-auto outline-0 bg-white dark:bg-slate-900 border border-solid border-slate-200 dark:border-slate-700 text-slate-900 dark:text-slate-300 min-w-listbox shadow-md dark:shadow-slate-900 [.open_&]:opacity-100 [.open_&]:scale-100 transition-[opacity,transform] [.closed_&]:opacity-0 [.closed_&]:scale-90 [.placement-top_&]:origin-bottom [.placement-bottom_&]:origin-top', |
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.
Adding custom selectors seems like the Tailwind way of doing this instead of using classes. This is probably blocked by the Tailwind plugin after v1, though, right? Just checking.
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.
This is a custom class, so its selector is not included in the Tailwind config we use.
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.
And what would you think about not using CssTransition
for the tailwind example and using the hooks directly instead? Wouldn't that be more idiomatic within tailwind users? This usage seems hard to find in the Tailwind docs
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.
Given my near-zero Tailwind experience, I don't know. @danilo-leal, I'd appreciate your input here, as you've used Tailwind more extensively than me.
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.
I have used arbitrary variant selectors with Tailwind very sparsely, but it doesn't feel too strange to me as the [...]
brackets API is pretty familiar at this point. But maybe it would be indeed a bit easier to parse if we were using hooks — assuming we'd be able to do something like expanded && 'classes'
.
(PS: Slightly off-topic, but I'm curious how this works with the ampersand being at the end 🤔 I'd intuitively assume it should be [&_.open]:
instead of [.open_&]
, matching the example from the doc link Diego shared.)
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.
But maybe it would be indeed a bit easier to parse if we were using hooks
Could we merge this PR as is and work on the hook demo separately?
I'm curious how this works with the ampersand being at the end
The open
class is applied to the parent element (popup), and we need to style the listbox conditionally depending on this class being present.
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.
Could we merge this PR as is and work on the hook demo separately?
Sure ✅
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.
Or we may end up redoing the demos if we decide to change the customization APIs
Replaced the Popper with Popup in the Menu implementation.
Similar to #40524, but with Menu.
Breaking change
slotProps.root
now accepts thePopupProps
instead of thePopperProps
Preview: https://deploy-preview-40731--material-ui.netlify.app/base-ui/react-menu/