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

feat(OverlayTrigger): allow renderProp pattern #5316

Merged
merged 1 commit into from Jul 23, 2020
Merged

Conversation

jquense
Copy link
Member

@jquense jquense commented Jul 20, 2020

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.

hideCallbacks.length = 0;
}

function useGlobalTimeout(
Copy link
Member Author

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.

@kosmiq
Copy link
Collaborator

kosmiq commented Jul 20, 2020

Super nice features!

I'll leave a proper review soon-ish. :)

Copy link
Member

@taion taion left a 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

www/src/components/NavMain.js Outdated Show resolved Hide resolved
src/useDelayToggleCallback.tsx Outdated Show resolved Hide resolved
src/useDelayToggleCallback.tsx Outdated Show resolved Hide resolved
src/useDelayToggleCallback.tsx Outdated Show resolved Hide resolved
@taion
Copy link
Member

taion commented Jul 21, 2020

btw is the global thing present upstream?

@jquense
Copy link
Member Author

jquense commented Jul 21, 2020

if you're in the "show" state, no delay in switching from one to the other

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

@jquense jquense changed the title feat(OverlayTrigger): allow renderProp and add global shared delays feat(OverlayTrigger): allow renderProp pattern Jul 23, 2020
@jquense
Copy link
Member Author

jquense commented Jul 23, 2020

Ok I split out the global timer stuff for now to not hold this up

@jquense jquense requested a review from taion July 23, 2020 16:50
@jquense jquense merged commit b2bf177 into master Jul 23, 2020
@jquense jquense deleted the overlay-improvements branch July 23, 2020 17:11
@syadav214
Copy link

UI is breaking for me on tooltip.

image

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

Successfully merging this pull request may close these issues.

None yet

4 participants