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

Add styles and render as dependencies for the position calculation effect #994

Merged
merged 3 commits into from
Mar 29, 2023

Conversation

gabrieljablonski
Copy link
Member

@gabrieljablonski gabrieljablonski commented Mar 28, 2023

Closes #993.

A down side might be that if styles is passed inline like this:

<Tooltip
  styles={{
    attr: value
  }}
/>

since the styles object will change on every render, the tooltip will recalculate its position on every render.

That doesn't seem like a big deal, but is there a smart way to avoid this, besides forcing the user to wrap the object with useMemo() or something similar?


https://stackblitz.com/edit/react-ts-rtgmca

  • add styles as a dependency for the calculation
  • watch for attribute changes on the component passed through the render prop

@akraines
Copy link

use a structural compare?

@gabrieljablonski gabrieljablonski marked this pull request as draft March 28, 2023 14:43
@gabrieljablonski
Copy link
Member Author

use a structural compare?

That could be an alternative, but at that point I'd rather leave it to the user to check themselves.

Btw, I thought this would fix #993 but I overlooked the fact you're using the render tooltip prop on your example, not styles. I'm working on solving the issue for that situation as well.

@gabrieljablonski gabrieljablonski changed the title Add styles as dependency for the position calculation effect Add styles and render as dependencies for the position calculation effect Mar 28, 2023
@gabrieljablonski
Copy link
Member Author

Test with [email protected]: https://stackblitz.com/edit/react-ts-rtgmca

This solution feels a bit cumbersome with having to forward the ref to the custom component, but not sure if there are any other viable ways of achieving this.

@gabrieljablonski gabrieljablonski force-pushed the fix/position-calc-styles-dep branch 2 times, most recently from 758bbd6 to 223b78d Compare March 28, 2023 21:24
@akraines
Copy link

akraines commented Mar 29, 2023

Worked like a charm - thanks for fixing this so quickly!
I think that the ref approach makes a lot of sense - I was actually thinking of recommending using react-measure.
In fact, that might still be a good idea - this way you could just wrap the result of the render with a measure [or your own resize observer like you did here] and then the user wont need to pass in the ref.

@gabrieljablonski
Copy link
Member Author

you could just wrap the result of the render with a measure [or your own resize observer like you did here] and then the user wont need to pass in the ref.

that's actually a great idea. I'll try it out whenever i get the time

@gabrieljablonski gabrieljablonski marked this pull request as ready for review March 29, 2023 17:23
@gabrieljablonski
Copy link
Member Author

With [email protected] no ref is required: https://stackblitz.com/edit/react-ts-rtgmca

@akraines thanks for the great tips. I believe using the wrapper shouldn't be a problem in 99% of cases, we'll see what we can do if it ever causes any issues.

Also you mentioning react-measure led me to the ResizeObserver API, which I forgot existed, and is a lot more elegant to use in this case over the MutationObserver API. So thanks again!

As soon as @danielbarion reviews it, we'll merge and do an official release.

Copy link
Member

@danielbarion danielbarion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good to me :)

@akraines
Copy link

Nice and clean. Thank you very much for taking the time to fix this

@gabrieljablonski gabrieljablonski merged commit aa70472 into master Mar 29, 2023
@gabrieljablonski gabrieljablonski deleted the fix/position-calc-styles-dep branch March 29, 2023 19:58
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.

Position is off if the content of the tootip's height changes
3 participants