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

Feature: Button asChild / Link asChild #552

Closed
wants to merge 11 commits into from

Conversation

thomasdigby
Copy link
Member

@thomasdigby thomasdigby commented Aug 21, 2023

This PR changes how the Link and Button components handle polymorphism. Instead of using as="a"/as="button" internally or externally, we can now use asChild and render the required element as children. This pattern helps with a bunch of typescript issues and retains the composability of styling and logic that we had previously. We use the Radix Slot component internally to merge any style, className, ref, onClick props, so we can compose Link with any child component.

I've also added logic for some useful (necessary?) attributes in the Link component if the href is not relative. This logic is currently handled by our "middleware" design-system components in atom-core, but they will eventually be removed so I moved it here. We lose the ability to do this with our Button components however, as we delegate responsibility for rendering the element to the user, so we can't repeat the same logic in Button. I think this is an ok compromise as the logic is not complex at all and the number of non-relative href "button"s we render is minimal.

There are ~100 instances of Link and Button components that will need to be updated in atom-core, but it should be a fairly simple migration. I have avoided updating ActionIcon with this logic as we're looking at deprecating the component soon, but I could look to update Tile if we like this approach.

<Button asChild css={{ mb: '$2' }}>
  <a href="https://google.com">Click here</a>
</Button>

// will render

<a href="https://google.com" class="c-fkUJsw c-fkUJsw-dXsdwv-size-md c-fkUJsw-gsycxF-cv c-fkUJsw-ijkLxnS-css">
  Click here
</a>
<Link asChild>
  <button onClick={() => console.log('yolo')}>Click here</button>
</Link>

// will render

<button class="c-fucOQC c-fucOQC-gvmVBy-size-md c-fucOQC-fmPQHp-noCapsize-true">
  Click here
</button>

With this pattern we get nicely merged css props, valid types and autocomplete on the child and parent, and composed onClick methods as expected

<Button
  asChild
  css={{ bg: 'red' }}
  onClick={() => console.log('bonjour')}
>
  <Box
    as="a"
    href="/yolo"
    css={{ bg: 'yellow' }}
    onClick={() => console.log('hello')}
  >
    Click me
  </Box>
</Button>

@thomasdigby thomasdigby marked this pull request as ready for review August 21, 2023 17:35
Copy link
Contributor

@Marcdup1 Marcdup1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool 😎


const buttonSpecificProps = href ? {} : { type }
if (asChild) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand how the loader will work if we are using the asChild option?

@thomasdigby
Copy link
Member Author

Closing this PR while we look into alternative options for improving our polymorphism

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.

7 participants