primitives icon indicating copy to clipboard operation
primitives copied to clipboard

`data-state` composition issue when composing multiple primitives with `asChild`

Open jjenzz opened this issue 3 years ago • 12 comments

Stemmed from this discussion https://github.com/radix-ui/primitives/discussions/560#discussioncomment-588638

When I did <Dialog.Trigger as={Tooltip.Trigger}> the data-state attribute was maintained from Dialog.Trigger instead of being overridden with the Tooltip.Trigger one. I'm not sure if this is a misunderstanding of as prop on my part (they're a confusing beast) but thought it worth looking into as a DX improvement if possible.

jjenzz avatar Apr 14 '21 14:04 jjenzz

Redundant now that we have asChild instead.

jjenzz avatar Sep 15 '21 11:09 jjenzz

In fact, the same applies with asChild... Looks like we need to update Slot to ensure that the data-state from the child takes precedence but Slot already overrides with child props so not sure what's going on here: https://codesandbox.io/s/eager-wood-dv0rl?file=/src/App.js

Ideally we would figure a way to have the state from both parts on the trigger still. There have been other discussions about this.

jjenzz avatar Sep 15 '21 11:09 jjenzz

A workaround is to control the component(s): https://github.com/radix-ui/primitives/discussions/560#discussioncomment-588638

benoitgrelard avatar Apr 05 '22 10:04 benoitgrelard

Hey guys,

I dive into it and found what's going on

when you have this structure

const DialogTrigger = (props) => {
   const Comp = props.asChild ? Slot: 'button'
   return <Comp {...props} />
}
const TooltipTrigger = (props) => {
   const Comp = props.asChild ? Slot: 'button'
   return <Comp {...props} />
}

<DialogTrigger asChild>
   <TooltipTrigger />
</DialogTrigger>

in the mergeProps function the slotProps will be the props passed to the <Comp /> in DialogTrigger and the childProps will be the props passed to the TooltipTrigger and not the <Comp /> underlying it and because hence childProps will be an empty object.

To get the <Comp /> props from TooltipTrigger it is possible to do children.type(children.props).props but I don't know if this is a good idea :upside_down_face:. With this, the child props will take precedence.

PS.: The issue occurs with all props. The events are not composed, for example. Only if you do:

onClick={event => {
  props.onClick(event)
}}

https://codesandbox.io/s/slot-x16b11?file=/src/App.tsx

joaom00 avatar May 23 '23 19:05 joaom00

Oh we know what the composition issue is, it's just that there is no obvious solution on how to best compose data-state whilst still maintaining good DX. Given that it only happens in specific cases, it hasn't been a huge priority but I agree it would be good to get down to it at some point.

Essentially, at the moment, there will always be one data-state that wins in the composition of 2 primitive parts that happen to have a data-state.

PS.: The issue occurs with all props. The events are not composed, for example. Only if you do: onClick={event => { props.onClick(event) }}

I don't understand what you're saying here?

benoitgrelard avatar May 23 '23 21:05 benoitgrelard

Hmm, I think I misunderstood the Slot behavior...

Just to be sure, given this example

<DialogTrigger asChild>
   <TooltipTrigger />
</DialogTrigger>

is the expected behavior that the props will merge between those of the rendered DialogTrigger element and the TooltipTrigger props (not the rendered TooltipTrigger element)?

joaom00 avatar May 23 '23 23:05 joaom00

Another good example is using something like a Toggle as the trigger for a Tooltip

<Tooltip.Root>
  <Tooltip.Trigger asChild>
    <Toggle.Root pressed />
  </Tooltip.Trigger>
</Tooltip.Root>

In this case, the data-state of the Toggle can be on|off. But the data-state of Trigger can be open|closed. Currently the data-state of the Trigger overrides that of the Toggle which means data-state can no longer be used to style a pressed Toggle.

Pearce-Ropion avatar Sep 01 '23 15:09 Pearce-Ropion

We just ran into the same issue (with Tooltip and Tabs). We thought one possible solution to this could be that data-state gets renamed to data-tabs-state and data-tooltip-state and so on, or that at least these scoped states are available in addition to data-state. I'd be happy to open a PR if a decision is made.

tiplutom avatar Jan 05 '24 13:01 tiplutom

Another good example is using something like a Toggle as the trigger for a Tooltip

<Tooltip.Root>
  <Tooltip.Trigger asChild>
    <Toggle.Root pressed />
  </Tooltip.Trigger>
</Tooltip.Root>

In this case, the data-state of the Toggle can be on|off. But the data-state of Trigger can be open|closed. Currently the data-state of the Trigger overrides that of the Toggle which means data-state can no longer be used to style a pressed Toggle.

I have this same problem with tooltips and toggles :'(

osnoser1 avatar Feb 20 '24 20:02 osnoser1

I think it will be better to append values to data-state. For the example like below,

<Tooltip.Root>
  <Tooltip.Trigger asChild>
    <Toggle.Root pressed />
  </Tooltip.Trigger>
</Tooltip.Root>

The data-state of Toggle.Root will be data-state="open on". In this case, we can still use the data-state in the css using selectors, like [data-state~='open'], [data-state~='on'].

chaejunlee avatar Feb 21 '24 01:02 chaejunlee

I have this same problem with tooltips and toggles :'(

Had this exact same issue. Solved a bunch of other issues related to composing tooltips/popovers/etc from this discussion, but then noticed my tabs weren't being styled when selected, because of data-state.

To fix this, in my tabs component I pulled out the selected tab state to a useState with value and onValueChange, then put my own "active" styling class on the button as appropriate. Wish I didn't have to do this.

vincerubinetti avatar Apr 30 '24 01:04 vincerubinetti