-
-
Notifications
You must be signed in to change notification settings - Fork 91
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
[useRenderer] 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 |
I've resolved all feedback and added a docs page: https://deploy-preview-1418--base-ui.netlify.app/react/utils/use-renderer |
docs/src/app/(public)/(content)/react/utils/use-renderer/page.mdx
Outdated
Show resolved
Hide resolved
docs/src/app/(public)/(content)/react/utils/use-renderer/page.mdx
Outdated
Show resolved
Hide resolved
docs/src/app/(public)/(content)/react/utils/use-renderer/page.mdx
Outdated
Show resolved
Hide resolved
docs/src/app/(public)/(content)/react/utils/use-renderer/page.mdx
Outdated
Show resolved
Hide resolved
Co-authored-by: Aarón García Hervás <[email protected]> Signed-off-by: Marija Najdova <[email protected]>
styleHookMapping: { | ||
excludedProp() { | ||
return null; | ||
}, | ||
}, |
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'm not the biggest fan of this prop name (styleHookMapping
) - not sure if "style hook" is suitable for a public API, and Map
instead of Mapping
might be better. It's not clear it infers/uses the state
object
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 most clear name would be: stateToDataAttributesMap
, to be very explicit, but it's a strange name 😅 @aarongarciah or @colmtuite do you have some suggestions?
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, "hook" has a strong different meaning in React world, so we can think of something else. stateAttributes
?
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.
stateDataAttributes
?
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.
Technically, you could create any attribute (including class
), so "dataAttributes" is not entirely correct. We just happen to use only 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 see! stateAttributes
makes sense then.
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.
Renamed to stateDataAttributes
, I am still missing the mapping in it, but people can figure out how to define them by using the type. Maybe it's even worth adding demo about it on the page. What do you think?
Closes #1315
Added a public
useRenderer
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:
Documentation page: https://deploy-preview-1418--base-ui.netlify.app/react/utils/use-renderer