-
Notifications
You must be signed in to change notification settings - Fork 399
feat(Button): allows button to render arbitrary component as its root #1891
feat(Button): allows button to render arbitrary component as its root #1891
Conversation
@asudoh @joshblack what do you think? |
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.
LGTM basically - Thank you for jumping in @ZacharyStair! Just one comment - Would you try merging the original <button>
handling code and <a>
handling code so your addition can fit well in there? BTW wanted to see with other reviewers on render
name, in case they come up with one.
Deploy preview for carbon-components-react ready! Built with commit f6b925b https://deploy-preview-1891--carbon-components-react.netlify.com |
src/components/Button/Button.js
Outdated
* Specify how the button itself should be rendered. | ||
* Make sure to apply all props to the root node and render children appropriately | ||
*/ | ||
render: PropTypes.func, |
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 proptype isn't strictly accurate; it could be either a function (i.e. a stateless functional react component), a React.component class, or even a string. it's complicated
edit: or maybe an Object, depending on what React.forwardRef
returns
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.
@zachary String is a fair point. Given typeof class {}
yields function
, probably PropTypes.oneOfType([PropTypes.func, PropTypes.string])
?
src/components/Button/Button.js
Outdated
Component = 'a'; | ||
} | ||
return ( | ||
<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.
Cool - Seems that we need to cover disabled
and type
for <button>
.
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.
fixed (sorry for the delay, i accidentally changed inputref
to inputRef
and was confused why all the tests were failing...)
the reason I picked edit: i guess react-router uses both a |
leverages a render prop to allow consumers to render whatever they want in place of button or anchor
closes #1890
7732644
to
6e7ff58
Compare
Hi there 👋. Could you talk me through a specific use case for this functionality? Thanks! |
@dakahn sure thing: I mentioned briefly in the linked issue: We're using the
renders
which renders
|
Oh -- sorry to make you repeat yourself. Reading it now you're quite clear! 😸 |
@ZacharyStair Instead of a For example: // Element
<Button as="a" />
// Component
<Button as={Link} /> Then the internal implementation uses |
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.
Just one additional comment - BTW I think as
makes sense to me, too.
src/components/Button/Button.js
Outdated
@@ -164,6 +173,7 @@ Button.propTypes = { | |||
}; | |||
|
|||
Button.defaultProps = { | |||
render: 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.
The default prop here seems to have caused some snapshot changes - Probably we can leave it undefined?
component = 'a'; | ||
otherProps = anchorProps; | ||
} | ||
return React.createElement( |
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 the jsx form might be clearer but either way works
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 seems that the main difference is createElement
automatically forwards refs, but we get an inputRef
instead of a ref
anyway so that part isn't strictly relevant
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.
LGTM 👍 - Thanks @ZacharyStair!
Forgot to include this before, but as a reference for that In this case, it uses |
🎉 This PR is included in version 6.98.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
leverages a render prop to allow consumers to render whatever they want in place of button or anchor
closes #1890