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

Issue with React Router v4 #89

Open
Ggayane opened this issue Mar 3, 2019 · 10 comments
Open

Issue with React Router v4 #89

Ggayane opened this issue Mar 3, 2019 · 10 comments

Comments

@Ggayane
Copy link

Ggayane commented Mar 3, 2019

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

Latest react still works with [email protected].

@pocketjoso
Copy link

pocketjoso commented Mar 4, 2019

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
Copy link
Collaborator

Redmega commented Mar 4, 2019

@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
Copy link

@Redmega Yeah using the portal api will work, I tested the basic functionality after making this minimal change:
pocketjoso@0f59ddd

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
Copy link
Owner

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

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

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
Copy link
Owner

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

Ggayane commented Mar 7, 2019

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

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

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

No branches or pull requests

4 participants