-
Notifications
You must be signed in to change notification settings - Fork 585
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
feat: ButtonWrapper prop in Button to customise rendering #41
Conversation
Awesome! Can you update the Button story so folks can find out about the usage? |
Oh, I added 2 stories, guess I haven’t pushed them |
That’s it :D I just pushed |
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.
First off @jsomsanith , thanks so much for looking into this & the speed of implementation. Much appreciated!
I pushed a small commit to this branch:
We have this helper that we are using for the Link
called StoryLinkWrapper
-- it is meant to represent a component that is similar to a React Router Link, Gatsby Link, Next Link, etc. When I update the new ButtonWrapper
anchor stories to use this component, we hit those errors that some non-standard anchor props are being passed down:
This only seems to happen w/ a
tags and to be honest, I am not sure why it didn't happen with the example styled.a
that you provided. That said, I have encountered these errors a lot w/ the Link
component and LinkWrapper
so I think it makes sense to look into them now before we merge this if that's ok.
I think it means having to do something similar to this:
https://github.com/storybookjs/design-system/blob/master/src/components/Link.js#L161-L167
Where you ensure that the props you need make it to the button styles (for appearance, loading state, etc.), but don't let them make it all the way down to the ButtonWrapper
. This fixed our issues w/ the Link
and LinkWrapper
so it should work here in theory as well, but def open to alternatives as well if you know of any.
@kylesuss fixed, and I added an optimisation: we memoize the styled button wrapper, instead of creating a new component at each render. Everytime it’s a new component, React considers it as a component change, the old will always be unmounted, and a new one mounted instead. |
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.
Looks great @jsomsanith ! Thanks for making those updates to suppress the annoying PropTypes errors and also for another great contribution.
const applyStyle = ButtonWrapper => { | ||
return ( | ||
ButtonWrapper && | ||
StyledButton.withComponent(({ containsIcon, isLoading, isUnclickable, ...rest }) => ( |
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.
Ah cool! Didn't realize you could do this same destructuring technique using withComponent
.
{buttonInner} | ||
</ButtonLink> | ||
); | ||
const StyledButtonWrapper = React.useMemo(() => applyStyle(ButtonWrapper), [ButtonWrapper]); |
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 idea w/ the memoization 👌
🚀 PR was released in v0.0.35 🚀 |
What is the problem this PR is trying to solve?
This PR resolve PR #36.
The Link component has a
LinkWrapper
feature, that takes a component to render, adding a Link style on it.We want the same feature for button.
What is the chosen solution to this problem?
Button
component now has aButtonWrapper
props, allowing to write that :This will render
CustomButton
/CustomAnchor
applying theButton
style