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

Allow customizing the mergeProps behavior for Slot #2631

Open
bengry opened this issue Jan 4, 2024 · 0 comments · May be fixed by #3083
Open

Allow customizing the mergeProps behavior for Slot #2631

bengry opened this issue Jan 4, 2024 · 0 comments · May be fixed by #3083

Comments

@bengry
Copy link

bengry commented Jan 4, 2024

Feature request

Add a prop or otherwise allow customize the way <Slot> handles prop merging. Specifically, it would be beneficial for tailwind users who often use https://github.com/dcastil/tailwind-merge to remove conflicting class names.

Overview

I'd like to be able to control the way className is merged. Right now the behavior is naive, resembling https://github.com/lukeed/clsx (but simpler). When using Tailwind, something like this would create a conflict:

// abbreviated implementation
function Overflow(
  { children, asChild }: { children: React.ReactNode, asChild?: boolean }
) {
  return (
    <Slot className="truncate break-normal text-md">
      {asChild ? children : <span>{children}</span>}
    </Slot>
  );
}

// usage

<Overflow asChild>
  <span className="text-sm">Excepteur Lorem adipisicing ut culpa commodo</span>
</Overflow>

This would end up rendering this HTML:

<span className="truncate break-normal text-md text-sm">
  Excepteur Lorem adipisicing ut culpa commodo
</span>

Here, text-sm and text-md conflict, and it's not clear who should/would win. When using tailwind-merge, the last class in precedence wins, in this case - the final rendered HTML should be this:

<span className="truncate break-normal text-sm">
  Excepteur Lorem adipisicing ut culpa commodo
</span>

Because it was provided on the child given to <Slot>.

Right now the only way to change this is to patch or fork the <Slot> implementation, replacing L117 with twMerge.

This in turn also means there are now two implementations of Slot in practice, given that someone uses more than just the Slot component from Radix - since other libraries depend on @radix-ui/react-slot as a peer dependency.

Two ways I thought about for solving this:

  1. Provide an optional prop for mergeProps, so one could use Slot as a primitive and build on of their own on top of it.
    a. Or maybe even just a classNameMerger (of type (parentClassName?: string, childClassName?: string) => string)
  2. Allow customizing the mergeProps for all <Slot> instances using an optional Context provider. Similar to how <Tooltip> can be globally configured.
    a. 1.a. above also applies here. I'm not sure what the performance cost would be for either option.

Examples in other libraries

None that I could find, but this blog post showcases how to implement a Radix-like <Slot> component, and touches on this very issue, incorporating tailwind-merge into their solution.

Who does this impact? Who is this for?

Tailwind users

Additional context

Related, but not the same: #1216, #2234, #2336

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants