-
-
Notifications
You must be signed in to change notification settings - Fork 93
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
[useRender] Add public hook #1418
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for base-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Nice ~ I imagine the use-case is something like this: https://codesandbox.io/p/sandbox/flamboyant-fog-9x266p?file=%2Fsrc%2FApp.tsx%3A29%2C63
(for some reason importing the hook in CSB doesn't work, this works fine locally in a playground though)
This feels more intuitive to me (biased though because I never used asChild extensively) to use than a Slot component
I haven't updated the exports field, will fix it. Here is the updated sandbox: https://codesandbox.io/p/sandbox/exciting-paper-9x266p-use-renderer-forked-ry269y?file=%2Fpackage.json |
It may be useful to provide a (alternative) way to opt-out individual props placed in state from generating a corresponding data attribute |
Thanks for the initial review, I updated the API to include:
|
I'd leave |
Yeah, fair enough, ok I was thinking initially to make the API simpler. I've changed it back to use the |
type Size = 'small' | 'medium' | 'large'; | ||
|
||
type TextState = { | ||
weight: Weight; |
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 isn't really the state of the component but its props, so someone might be confused about the purpose of State and style hooks. Can we introduce something else that doesn't come from props? For example, a character counter?
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 added color inside this too, which is a state that changes when clicking the text. I am out of ideas what else to include, maybe we can have a demo with a different component, more complex to showcase these features. I felt character counter, although could be state, it's not really valuable while styling.
Co-authored-by: Michał Dudak <[email protected]> Signed-off-by: Marija Najdova <[email protected]>
Fine to me pending changing the name to useRender as per discussion. 🚀 |
<Text className={styles.Text}>Text component rendered as a paragraph tag</Text> | ||
<Text className={styles.Text} render={<strong />}> |
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'd be more realistic if the component set its own class name instead of passing a class name to each instance. This also means removing className
from TextProps
.
<Text className={styles.Text}>Text component rendered as a paragraph tag</Text> | |
<Text className={styles.Text} render={<strong />}> | |
<Text>Text component rendered as a paragraph tag</Text> | |
<Text render={<strong />}> |
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.
For sure 👍
|
||
### Supporting the `render` prop | ||
|
||
This is an example of a Text component that provides the support for the render prop. |
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.
For consistency, since the heading before this paragraph uses backticks.
This is an example of a Text component that provides the support for the render prop. | |
This is an example of a Text component that provides the support for the `render` prop. |
.Text { | ||
font-size: 0.875rem; | ||
line-height: 1rem; | ||
&[data-size-small] { | ||
font-size: 0.75rem; | ||
} | ||
&[data-size-large] { | ||
font-size: 1.25rem; | ||
} | ||
} |
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'd simplify this codeblock by removing unrelated code and nesting.
.Text { | |
font-size: 0.875rem; | |
line-height: 1rem; | |
&[data-size-small] { | |
font-size: 0.75rem; | |
} | |
&[data-size-large] { | |
font-size: 1.25rem; | |
} | |
} | |
.Text[data-size-small] { | |
font-size: 0.75rem; | |
} | |
.Text[data-size-large] { | |
font-size: 1.25rem; | |
} |
font-size: 0.875rem; | ||
line-height: 1rem; | ||
color: var(--color-gray-900); | ||
} |
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 an example of a Text component that provides the support for the render prop. | ||
|
||
<Demo path="./demos/render" /> |
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 think this example makes it look more complex than it really is, due to imports, types, styles, etc. I wonder if it'd be better to just have a code snippet and not a live example for this. Something like:
This is an example of a Text component that provides the support for the render prop. | |
<Demo path="./demos/render" /> | |
This is an example on how to create your own `render` prop: | |
```jsx | |
import * as React from 'react'; | |
import { useRender } from '@base-ui-components/react/use-render'; | |
function Text({ render, ...otherProps }) { | |
const { render, ...otherProps } = props; | |
const { renderElement } = useRender({ | |
render: render ?? <p />, | |
props: otherProps, | |
}); | |
return renderElement(); | |
} | |
``` | |
It'd be used as follows: | |
```jsx | |
<Text>I'm a paragraph</Text> | |
<Text render={<span />}>I'm a span</Text> | |
``` |
The downside is not being able to open it on CodeSandbox, but not sure if it's that important.
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.
It could be an (another) argument for having the ability to select code to display in a demo preview. This way we could have the best of both - a concise code snippet that's runnable and can be opened in CSB. I could take a look at this next if you agree this could be handy.
The downside is not being able to open it on CodeSandbox, but not sure if it's that important.
An (IMO more serious) downside is that contrary to demos, code snippets are not verified in any way (not executed, linted, typechecked, etc.), so they could be invalid (or inadvertently become invalid in the future after changes to the API).
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.
It could be an (another) argument for having the ability to select code to display in a demo preview
This would be perfect.
code snippets are not verified in any way (not executed, linted, typechecked, etc.), so they could be invalid (or inadvertently become invalid in the future after changes to the API)
Yeah, it's a trade-off. Sometimes live demos are an overkill and makes things harder to convey. I'll leave the live demo for now.
|
||
### Generating data attributes | ||
|
||
In the following demo, the Text component provides more Base UI features, like adding data attributes and `className` callback where developers can have access to the internal state of the component. |
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.
In the following demo, the Text component provides more Base UI features, like adding data attributes and `className` callback where developers can have access to the internal state of the component. | |
`useRender` can also be used to automatically add data attributes based on the component's internal state and support a callback in the `className` prop where developers can access the component's internal state. |
It's weird that the paragraph talks about the className callback but the it's not present in the demo.
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.
Maybe we can extend the demo on the usage to show className callback where a class is added fort he large variant. What do you think?
}); | ||
``` | ||
Then, developers can target the data attributes like this: |
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.
Seeing data-size-small
might confuse users into thinking the generated data attributes have this shape by default (instead of data-size='small'
). I'm not sure if this paragraph and the code block below are necessary.
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.
The demo is about showcasing that you can override the default behavior data-size='small', to generate data-size-small
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.
Maybe we need a better description of what it is about.
|
||
<Demo path="./demos/render" /> | ||
|
||
### Generating data attributes |
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 think we could come up with a better demo. Clicking a text is not very common and it's hard to discover. We could also simplify it and only leave one "state" (color or size, but not both).
We could also use this demo only for data attributes and leave the className callback as paragraph + code snippet in a new section. This could also improve the discoverability of the className callback 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.
Yeah, the thing I was trying to add here is that people can use internal state as well, I couldn't come up with an idea for internal state for Text component :) I am ok with simplifying it.
Closes #1315
Added a public
useRender
hook as an simpler adapter that uses theuseComponentRenderer
hook. I intentionally copied the types, so we make sure we don't break them if we ever change the internaluseComponentRenderer
hook.The hooks receives the following settings:
customStyleHookMapping
)Documentation page: https://deploy-preview-1418--base-ui.netlify.app/react/utils/use-render