search

adobe / react-spectrum

A collection of libraries and tools that help you build adaptive, accessible, and robust user experiences.
https://react-spectrum.adobe.com
Apache License 2.0
12.55k stars 1.09k forks source link

`Tooltip` doesn't receive `slot` props from `Provider` #6802

Closed ArrayKnight closed 1 month ago

ArrayKnight commented 1 month ago

Provide a general summary of the issue here

I'm trying to use the Provider to supply a default for the placement prop so that child components have default placement that best aligns with the layout of my parent component.

However, it appears that the Tooltip component doesn't merge props in from context slots as expected

🤔 Expected Behavior?

Props provided from Provider context are passed through slots per the established pattern in other components

😯 Current Behavior

Tooltip doesn't merge in props and props must be implemented locally

💁 Possible Solution

Looking at the implementation of the TooltipTrigger, it looks like it is overriding this context: https://github.com/adobe/react-spectrum/blob/826d9d450ab075f6e7aa6b4cdcda944b19b12acb/packages/react-aria-components/src/Tooltip.tsx#L92

One possibility would be to utilize a MergeProvider: https://github.com/adobe/react-spectrum/issues/6610 allowing for the props to flow through without being completely replaced

🔦 Context

I have a component that is aligned in a way where establishing a default placement for tooltips based on its own layout is desirable

🖥️ Steps to Reproduce

https://codesandbox.io/p/sandbox/heuristic-sun-gk9v85

Version

"react-aria": "3.33.1", "react-aria-components": "1.2.1", "react-stately": "3.31.1",

What browsers are you seeing the problem on?

Chrome

If other, please specify.

No response

What operating system are you using?

Mac

🧢 Your Company/Team

No response

🕷 Tracking Issue

No response

snowystinger commented 1 month ago

I think you might've forgotten to save your codesandbox?

ArrayKnight commented 1 month ago

Apologies, I've updated the link

snowystinger commented 1 month ago

Our guidance for scenarios like this has been to create your own context to pass your design system specific props through as you're likely not going to be exposing RAC's Tooltip directly, it'd be "YourDSTooltip as Tooltip" or something like that.

You could also do it by wrapping the TooltipTrigger and putting a context merger as a child of it with the props you want.

One thing to note is that we've actually found the merging logic, such as mergeProps, to be a possible bottleneck/slowdown. So I think we'd want to be very careful about adding more merging.

ArrayKnight commented 1 month ago

Can do. Thanks for the guidance

I'll be interested to know how y'all plan to tackle that potential bottleneck considering how prevalent mergeProps is throughout your codebase

ArrayKnight commented 1 month ago

@snowystinger There's a PR open if you're available to review

snowystinger commented 1 month ago

Closing, see https://github.com/adobe/react-spectrum/pull/6820#issuecomment-2276951035 for details

ArrayKnight commented 4 weeks ago

@snowystinger I've posted comments and a commit for your consideration that may help to address the edge case you brought up