-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[POC] Slots using render props #38362
Conversation
Netlify deploy previewhttps://deploy-preview-38362--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
Is the render prop option different from this?: <MenuButton
slots={{
root: (props) => (
<IconButton data-testid="hamburger-menu" {...props}>
<MenuIcon />
</IconButton>
),
}}
/> |
I remember we had a similar discussion when working on Fleunt UI (apparently 5 years ago 🤯). Some of the things discussed are very well summarized in microsoft/fluent-ui-react#199 (comment). It is definitely not a silver bullet. We must be careful around performance with this one too or at least to check if it will regress compared to plain components. One more thing, if this component is later exported from a UI library like a component, the merging of all props and event handlers would be nightmare. It would be great if we can create a fully featured introduction demo and see how it would look like. |
@DiegoAndai, yes, there is a subtle but very important difference. A render prop is a function that returns JSX (it's called by us), while
Thanks for referencing this. I'll analyze what you found back then.
Is there something in particular that you are concerned with? I don't think performance will be much different between these solutions. When using
For creating reusable libraries with Base UI, we recommend using hooks. Even today it's hard to create components that can be customized further with Base UI's components (merging ownerStates of different shapes is one example of why it's hard).
Do you mean a demo of the MenuButton integrated with a Dropdown and a Menu? |
I thought the point of the hooks is to have more freedom in how the DOM tree is rendered & structured. If we truly think that the unstyled components are not suitable for creating UI libraries, then what's the use-case for them? I mean even if I am not creating a UI library, but I need to have two buttons in my app, and I wrap the ButtonUnstyled once, for sure I would like to reuse this component in the other place - this doesn't mean that I shouldn't have access to slots & slotProps in this other instance. I don't think we should box ourselves like this honestly, otherwise, we may be wasting time with the components.
It's connected to the previous point, maybe our components are not tested well enough for us to know if the API works. The hooks are used in both Joy UI and Material UI so we can tell if they work or not, the demos are the only way how we can test the unstyled components. This is why I said, maybe we should have a fully featured Introduction demo - create a component that can be used in multiple places based on the unstyled component - this will show us if the API works. |
My concern is that we're making the Radix's composition API has one pattern to achieve this and have very specific rules on how to do so. This makes the documentation easier to understand. Another point regarding components and hooks: React Aria went the hook route and stuck with it, Radix went the component route and stuck with it. Having the components, and the hooks, and |
FWIW, this is a common complaint with regards to customization in Material UI. Because there are so many different ways to do it (and our docs don't explicitly teach one way as the best option), devs feel major "analysis paralysis" and have a hard time deciding which to choose—and then what they take away from the experience is that "Material UI is too difficult to customize." 🫠 I could imagine similar situations here. |
Components can be easily customized as long as you don't add anything to ownerState - then it becomes cumbersome. We can discuss use cases for both separately, not to divert from the original topic of this discussion.
All right, I'll work on it.
@DiegoAndai, @samuelsycamore, I'm not considering adding render props to the existing features but replacing slots and slotProps with them. |
That makes more sense. In that case, I agree with @DiegoAndai 's other point, that it kind of muddies the concept of slots. It's not necessarily a bad thing, but it would require a shift in how we present the mental model of component structure and customization. Maybe it's just a matter of naming, but if I'm understanding it correctly, it seems to add an extra layer of abstraction on top of our already abstracted concept of slots. Or would this replace the concept of slots entirely? |
TL;DR: Consider not having slots nor render props, instead encourage using the hook as soon as the user needs more customization. (Sorry, this came out longer than expected 😅) Making our users' life harderI think the underlying discussion here is the narrative we will have regarding components vs. hooks. When should the user use the component? When should the user use the hook? Where's the line between the two? If we don't have that clear, our users will be confused, and we will make the DX worse from the start. IMO, having the render prop (or the slots) makes the line blurry. We add overlap between the component and hook use cases, delegating the decision to the user who's not an expert on the product. We're the experts on the product, we should aim to make the most decisions so the way to use it is as clear as possible. Our current narrative looks like this:
In point 2. we introduce the "analysis paralysis" @samuelsycamore mentioned. The user's goal is to create their custom component and we're adding complexity to that goal. TailwindOne of Tailwind's strong points is that they chose one simple API: if you need a style, add this class to your element. They didn't provide abstracted classes, instead they encourage extracting components (or templates). They even discourage CSS abstractions. I would say they simplify the user decision to:
There's a clear answer for each need, and they provide guides on how to move from 1. to 2. Sure, there are people who are against using utility classes like this, but most users just want to style their project and don't care about the nuances between using a utility class or plain CSS. Making our users' life easierI say there's a third option regarding slots / render props: not having either. Maybe I'm missing something, but the value the component provides is the structure, so why would we have a prop to override that structure? So our narrative for the users could be:
And we should provide a guide on how to move from 1. to 2. Sure, there will be people who care about the difference between using slots, a render prop, or a hook, but most won't, most just want to build their component and move on to other things. There's even a fourth option, which was the one Radix took: just having components. For me that's not the best solution, hooks are way better for total flexibility. Maybe people are not used to using hooks for these things, but as it says on Tailwind's landing: "If you can suppress the urge to retch long enough to give it a chance, I really think you’ll wonder how you ever worked with BenefitsSummarizing the options:
I think if we're more opinionated and choose option 3. we will:
|
@DiegoAndai React Aria is adding unstyled components, they are in alpha. Two more things to be considered for the callback scenario:
|
<CssVarsProvider> | ||
<WithRenderProp /> | ||
<WithSlotsAndSlotProps /> | ||
<WithSlots /> |
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 accidentally noticed that the WithSlots
demo doesn't behave like the other 2 examples:
Screen.Recording.2023-10-03.at.18.29.40.mov
Screen.Recording.2023-10-03.at.18.29.26.mov
The first 2 examples render the Menu in a popper, while the 3rd one doesn't.
triggerElement
seems to be null
for some reason:
if (open === true && triggerElement == null) { |
@michaldudak any ideas?
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.
Good catch! The issue is that MenuIconButton does not forward the ref. I'll fix 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.
Ahh, thanks for the explanation!
The drawbacks I see:
I would stay with the current API (slot, slotProps) and live with the trade-off (maybe 10% of the use cases which is fine to me). What we need is to document them and teach the community when to use those props. |
Let's discuss the render props API for slots.
The implementation is quick and dirty and is there only to enable us to play with the new API.
Following up on the discussion we had on the Monday meeting, I prepared a version of the MenuButton with the added
renderRoot
prop. It's called with the props the root component should receive and the ownerState. Developers can override the component or modify the props in one place (so it's likeslot
andslotProps
combined).Codesandbox playground: https://codesandbox.io/s/heuristic-hodgkin-74mlzx?file=/Demo.tsx
🥇 The upsides:
slotProps
).component
prop lost when wrapping withstyled
#29875👎 The downsides:
slotProps
merge them). This could be solved by a function that merges both sets of props.Original discussion about the topic: #21453