Skip to content
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

chore(@kadena/react-ui): Updated the Link component to work with external links #834

Merged
merged 9 commits into from
Aug 31, 2023

Conversation

eileenmguo
Copy link
Contributor

@eileenmguo eileenmguo commented Aug 29, 2023

There is been some confusion about how to use react-ui components with Next/Link. Previously many different solutions have shown up across the components, but this PR is the result of an effort to create one convention for this issue.

This approach allows users to set an asChild prop on the Link component and add the external component as a direct child. When this prop is set, all props from the Link component are passed down to the child along with styles and icons.

It can be used like so:

<UILink icon="Account" asChild>
  <NextLink href="...">
    Link Label
  </NextLink>
</UILink>

The benefit of this is that the semantic html ends up with only one anchor and it keeps the styles and logic within the ui library's Link component

Caveats:

  • href prop on the UI's Link component is now optional so that users can set an href on the child. This means that users can potentially use the Link component without an href and there won't be a typescript error. I think this is still better than having to require the user to set an href on the Link component and the external component
  • In the case of the same props being set on both the Link component and the external component, the external component props will override the ones applied to the Link component

The idea is that we update all link components to allow for this. Curious to know what you think!

UPDATE
It looked like no apps were using the Link component, so I updated the Breadcrumb component as well to check how it would look in other applications. I went ahead and removed the string interpolation on the href from the base react-ui Breadcrumb component to the docs and updated the implementation with this new approach. The result is cleaner html!

@changeset-bot
Copy link

changeset-bot bot commented Aug 29, 2023

🦋 Changeset detected

Latest commit: e8f7bb8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Aug 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
alpha-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 31, 2023 8:11am
docs-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 31, 2023 8:11am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
immutable-records ⬜️ Ignored (Inspect) Visit Preview Aug 31, 2023 8:11am
react-ui ⬜️ Ignored (Inspect) Visit Preview Aug 31, 2023 8:11am
tools ⬜️ Ignored (Inspect) Visit Preview Aug 31, 2023 8:11am

@timoheddes
Copy link
Contributor

I feel the best way to really review this and see if it suits our needs the way we hope, is through actually field testing it and implementing it. Should we update components that currently support Next Links so that they use this? I'm guessing that would be the idea anyway and then we can see if we run into other issues or not?

@eileenmguo eileenmguo merged commit 6119906 into main Aug 31, 2023
5 checks passed
@eileenmguo eileenmguo deleted the chore/react-ui-update-link branch August 31, 2023 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants