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

data-state composition issue when composing multiple primitives with asChild #602

Open
jjenzz opened this issue Apr 14, 2021 · 11 comments · May be fixed by #2716
Open

data-state composition issue when composing multiple primitives with asChild #602

jjenzz opened this issue Apr 14, 2021 · 11 comments · May be fixed by #2716
Labels
Has Workaround This issue isn't fixed but has a workaround

Comments

@jjenzz
Copy link
Contributor

jjenzz commented Apr 14, 2021

Stemmed from this discussion #560 (comment)

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
Copy link
Contributor Author

jjenzz commented Sep 15, 2021

Redundant now that we have asChild instead.

@jjenzz jjenzz closed this as completed Sep 15, 2021
@jjenzz
Copy link
Contributor Author

jjenzz commented Sep 15, 2021

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 jjenzz reopened this Sep 15, 2021
@benoitgrelard benoitgrelard added Has Workaround This issue isn't fixed but has a workaround Resolution: Backlog labels Apr 5, 2022
@benoitgrelard
Copy link
Collaborator

A workaround is to control the component(s): #560 (comment)

@benoitgrelard benoitgrelard changed the title Figure out why as={Component} doesn't use data-state from Component data-state composition issue when composing multiple primitives with asChild Apr 5, 2022
@joaom00
Copy link
Contributor

joaom00 commented May 23, 2023

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 🙃. 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

@benoitgrelard
Copy link
Collaborator

benoitgrelard commented May 23, 2023

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?

@joaom00
Copy link
Contributor

joaom00 commented May 23, 2023

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)?

@Pearce-Ropion
Copy link

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.

@tiplutom
Copy link

tiplutom commented Jan 5, 2024

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.

@osnoser1
Copy link

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 :'(

@chaejunlee
Copy link

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'].

@vincerubinetti
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has Workaround This issue isn't fixed but has a workaround
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants