search

romainberger / react-portal-tooltip

Awesome React tooltip
http://romainberger.github.io/react-portal-tooltip/
MIT License
405 stars 73 forks source link

Issue with React Router v4 #89

Open Ggayane opened 5 years ago

Ggayane commented 5 years ago

Whenever I put <Link> inside the tooltip I get this error: You should not use <Link> outside a <Router>

Can you please tell how to fix the this?

pocketjoso commented 5 years ago

Latest react still works with react-portal-tooltip@2.4.0.

pocketjoso commented 5 years ago

I believe it was this change that broke it. I.e. changing from using React.unstable_renderSubtreeIntoContainer to doing a completely separate ReactDOM.render. Of course the latter will not work, as the react-router parents are not part of the new react-tree that is created.

I don't know the case for sure, but should ReactDOM.createPortal be used rather than ReactDOM.render?

Will have to fork to fix this for my use case; if portal work I will create a PR here.

Redmega commented 5 years ago

@pocketjoso It's caused because it renders outside of the root, so yes you are correct, the router is not in the parent dom tree according to the tooltip.

using the createPortal api might work. Update here once you've had the chance to experiment with it -- I'll be sure to review the PR.

pocketjoso commented 5 years ago

@Redmega Yeah using the portal api will work, I tested the basic functionality after making this minimal change: https://github.com/pocketjoso/react-portal-tooltip/commit/0f59dddf41235eafdcfa643a396e121d56725bdf

However I feel hesitant to do a PR for it, as some more code needs to be refactored. This is because the portal rendering now happens in the (react) render function, rather than in your own renderPortal class method. I believe the latter method should be removed completely now, but first the code responsible for only hiding the tooltip after the given tooltipTimeout option would need to be rewritten: https://github.com/romainberger/react-portal-tooltip/blob/master/src/index.js#L419-L425 it needs to work a bit differently now, probably setting state.

Do you reckon you can push a fix for this instead?

romainberger commented 5 years ago

The portal api works but does not produce the same result as the current implementation. Currently a single tooltip is created for all the parents, where the portal api will create multiple tooltip, so it breaks the idea of a single tooltip moving.

pocketjoso commented 5 years ago

True. Is it more important to you to preserve that functionality than to allow the components to stay in the same react dom tree? I'm not sure going forward that these two separate goals will ever be possible for a single component to satisfy at the same time. So would be good to know what the stance is for this library, as this issue is a deal-breaker for us.

pocketjoso commented 5 years ago

I guess potentially it could be an option in the library, so the consumer could chose what is important to them, but it would make the code a bit heavier and more messy.

romainberger commented 5 years ago

I initially made this component exactly for this behavior of a single floating tooltip, so I would definitely keep it that way. I understand the lack of context is an issue and there should be a search for a fix if possible. But if it's not possible and it means the death of this component, than I'd rather see it deprecated than transformed into yet-another tooltip component. I think there are enough of these available to fill every need.

I'm sorry if this means you'd have to migrate to another library, unfortunately I can't for now work on a fix, so I can't say when it will be fixed, if ever.

Ggayane commented 5 years ago

I initially made this component exactly for this behavior of a single floating tooltip, so I would definitely keep it that way. I understand the lack of context is an issue and there should be a search for a fix if possible. But if it's not possible and it means the death of this component, than I'd rather see it deprecated than transformed into yet-another tooltip component. I think there are enough of these available to fill every need.

I'm sorry if this means you'd have to migrate to another library, unfortunately I can't for now work on a fix, so I can't say when it will be fixed, if ever.

I can fully understand you vision and you are right, keeping this as a simple tooltip is a good idea, I opened this issue and then asked myself but why it would do that?! 😄

And thank you for this library!

pocketjoso commented 5 years ago

Yeah thanks for explaining your position - it helps. And thanks as well for providing the library! 👍