-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat(OverlayTrigger): allow renderProp pattern #5316
Conversation
5503071
to
7960405
Compare
src/useDelayToggleCallback.tsx
Outdated
hideCallbacks.length = 0; | ||
} | ||
|
||
function useGlobalTimeout( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is extremely inelegant, open to simplifications.
Super nice features! I'll leave a proper review soon-ish. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not quite sure what the "global" bit is supposed to do. can you explain? it looks a little odd – it looks like e.g. everything will fire hide at the same time when the first thing finishes hiding?
are we looking for something like:
- there's a global show/hide state with the timer to transition in/out
- if you're in the "show" state, no delay in switching from one to the other
i think you want some sort of ref-like thing to generate some sort of "key" or something per element?
not sure... also from an API POV, the combination of global with the implicit state + possibly controlled things is... sorta weird. like, they could go out-of-sync, but the "global"-ness is just a hidden, background thing.
you almost want to see more of a global show/hide context provider that just, like, replaces the uncontrolled behavior for things inside the context running on this, with something like a special show='@@global'
or something
btw is the global thing present upstream? |
That's the core of it yes, Once a tooltip is shown you want the next one to show immediately until an normal delayed exit occurs. I threw this together very quickly b/c it was annoying me and i was hoping i could avoid a more involved API. something like: tippy or the reach-ui tooltip I think it's ok UX to not have more local or nuanced scoping, like a provider, but I agree that as is it's ugly and bad. Like you probably also want to skip the animation for already entered tips but a good way to do that didn't seem obvious. Maybe what you actually want to do is share a single tooltip component and change the content and reference, but that seems hard... |
7960405
to
a0c159c
Compare
a0c159c
to
87b2cd0
Compare
Ok I split out the global timer stuff for now to not hold this up |
Adds 2 features. First, OverlayTrigger render prop for more nuanced positioning situations (added docs example). Second, adds a global shared delay for tooltips in toolbars.