-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
refactor: connect ShadCN Link
to ShadCN ButtonLink
#13560
refactor: connect ShadCN Link
to ShadCN ButtonLink
#13560
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@pettinarip replaced the Override story with one for ButtonLink (separate story file) |
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! good job @TylerAPfledderer 💪🏼
<ChildContent {...props} size={size} isIconLeft={isIconLeft} /> | ||
<ButtonLink | ||
className={commonClassStyles} | ||
// size={size} |
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.
Did comment this out to skip a type error that was breaking the build. I see this as a minor issue for now since this component is not currently used. We can tackle the issue in a separate PR.
I also think that <ButtonTwoLines componentType="link" />
is not working. Something we can look into later.
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.
With componentType
, the type guarding is not working as expected. Need to re-evaluate the approach.
With size
, it is housed in buttonProps
when passing props to ButtonLink
. Not going to try and make the approach any more strict, but can simply run a merge props of buttonProps
with size
.
if (props.componentType === "link") {
const { buttonProps, ...rest } = props
const mergedButtonProps = merge(buttonProps, { size })
return (
<ButtonLink
className={commonClassStyles}
buttonProps={mergedButtonProps}
{...rest}
>
<ChildContent
{...rest}
size={size}
isSecondary={buttonProps?.isSecondary}
isIconLeft={isIconLeft}
/>
</ButtonLink>
)
}
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.
I think we should follow the same approach we took with the original ButtonLink. In this case, I think we should return just a button to simplify everything. Then, in case you need a buttonlink, the user/dev could do the same we are doing with the buttonlink.
// always a button
<ButtonTwoLines size=".." variant="..." />
// in case you want a buttonlink => compose them with asChild (same as with ButtonLink)
<ButtonTwoLines asChild>
<BaseLink> ...
</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.
@pettinarip Good with me! Getting too DRY here, and it's showing its cracks
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.
👍🏼
Syncs the updated
Link
andButtonLink
components with ShadCN.text-background
utility in the "solid" style variant no longer needs theimportant
flagactiveClassName=""
as this override might still be needed for active page links.