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 REQ] Dynamic callback for disabling tooltip #1210

Closed
nareshpingale opened this issue Jul 20, 2024 · 9 comments · Fixed by #1211
Closed

[FEAT REQ] Dynamic callback for disabling tooltip #1210

nareshpingale opened this issue Jul 20, 2024 · 9 comments · Fixed by #1211
Labels
Awaiting merge Issue is fixed on a PR that will me merged soon. Feature Request

Comments

@nareshpingale
Copy link
Contributor

In my application, I work with large datasets displayed in a data grid, where each cell has a tooltip. This extensive use of tooltips results in significant scripting time whenever a user interacts with them. The addEventListener and removeEventListener functions run on every cell and tooltip across the app. Consequently, even simple actions like scrolling or hovering through different cells through the table cause a considerable amount of scripting activity, impacting performance.

Currently, I've implemented a workaround tailored to my use case. Tooltips are necessary only when the data within a cell is clipped and an ellipsis appears. However, this solution is limited and not ideal for broader scenarios as it helps only because it reduces the amount of tooltips instead of solving the core problem.

I propose adding a data-tooltip-dynamic-hide attribute that accepts a callback function.

This callback would:

  • Receive the anchor element and anything else that you think would be ideal exposing outside to the users from inside of Tooltip.tsx
  • User would add logic using this callback. In my case compares the scrollWidth and offsetWidth properties to determine if the content is clipped.
    data-tooltip-dynamic-hide={(anchorElement)=>anchorElement.scrollWidth <= anchorElement.offsetWidth}
  • Decide whether to add the element as an anchor for the tooltip based on the result inside Tooltip.tsx

This approach would allow for more flexible and efficient tooltip management, accommodating various use cases and improving overall performance.

Alternatives I have explored for far

  • I have explored using data-tooltip-hidden attribute. But access correct ref and in such cases inside .map become troublesome.
  • As temporary workaround I have added show delays as to act as some kind of throttle.
  • Considered a broader fix adding throttle over useEffect callback which adds and removes listeners.

Additional context
I will be happy to open a PR introducing this feature or discussing other solutions around this.

Tooltips added on most basic example of tanstack table.
https://codesandbox.io/p/sandbox/angry-night-32nr8q

image
@gabrieljablonski
Copy link
Member

gabrieljablonski commented Jul 20, 2024

I'll take a deeper look when I got some more time, but IIRC, your suggestion is not exactly viable. I believe a data-* can only really be a string (see here).

For instance, we currently do support data-tooltip-html, which requires "encoding" the JSX into a string, but I'm not sure doing that with a function is a good idea (although possible, I think?).

I'll try to come up with something to address your problem (the first thing that comes to mind is virtualizing your table), but I'm not sure there's anything we can do easily internally with the tooltip.

@nareshpingale
Copy link
Contributor Author

nareshpingale commented Jul 21, 2024

You are correct that data-* attributes must be strings. I considered using a callback to make the solution more generic. However, my current implementation is with data-tooltip-dynamic="true" and hardcode the logic for anchorElement.scrollWidth <= anchorElement.offsetWidth directly in Tooltip.tsx

In my current application, I am already using virtualization. Despite this, with 50 rows and 10 columns, the grid alone generates 500 tooltips. Additionally, there are other virtualized lists that add even more tooltips at any given time.
I used a basic example to clearly illustrate this scenario, which also has just 132 rows and 6 columns. If we just take this example into account, virtualisation shouldn't be solution for such a small table.

Please let me know if my use case seems reasonable and if you think it requires and is worth your and my time. In my opinion, any medium to large-scale application can easily reach this number of tooltips, and having to add and remove event listeners along with other operations for all of them raises performance concerns.

@gabrieljablonski
Copy link
Member

gabrieljablonski commented Jul 21, 2024

I'm convinced this is a feature that is worth looking into.

We might not be able to do it through a data-* attribute, but it could be a regular tooltip prop. For example, we already have the render prop, which exposes the ref for the anchor element, but it does not solve this issue, since render happens after the event listeners were already attached.

Something like this could work nicely. It's literally just your suggestion, but as a prop instead.

<Tooltip
  // Using the "overflowing text" logic from your use-case.
  disableAnchor={anchorRef => anchorRef.scrollWidth <= anchorRef.offsetWidth}
/>

Just not sure about calling it disableAnchor, and also calling it "dynamic hide" doesn't accurately represent what would actually be happening internally in the tooltip.

@nareshpingale
Copy link
Contributor Author

nareshpingale commented Jul 21, 2024

How would this work if this is not the expected behaviour for all tooltips across the app ? Are you suggesting we add a separate <Tooltip with a new id and add this prop there ? Which also means a new <Tooltip for new conditions.

Although as a workaround I can add some HTML data attribute as an identifiers and read them inside the callback for making decisions.

@gabrieljablonski
Copy link
Member

Although as a workaround I can add some HTML data attribute as an identifiers and read them inside the callback for making decisions.

Yep. Your idea of having something like data-tooltip-dynamic="true" is still valid, since you have access to the anchor ref from the callback.

I wouldn't call it a "workaround", since the intention of this callback is to have the decision logic completely outside the tooltip code. Internally, we'll just call disableAnchor(anchorRef), and skip adding the event listeners if that returns true.

@nareshpingale
Copy link
Contributor Author

nareshpingale commented Jul 21, 2024

Sounds perfect, thank you for entertaining my request and immediate responses. I will be happy to drop a PR on this for you to add corrections if you wish so.

@nareshpingale
Copy link
Contributor Author

nareshpingale commented Jul 23, 2024

@gabrieljablonski just curious if we are dealing with the core problem of having so many event listeners in V6 ? Me and my team are exploring the possibility of building and implementing a custom tooltip component for replacing react-tooltip's basic usecases in my app, which would be having a single event listener following event delegation pattern.

@gabrieljablonski
Copy link
Member

gabrieljablonski commented Jul 23, 2024

just curious if we are dealing with the core problem of having so many event listeners in V6?

having a single event listener following event delegation pattern.

The idea of event delegation might be an interesting approach, but we have to investigate if that allows us to keep the tooltip functionality mostly the same, and if that does in fact improve performance.

For instance, let's say we use body/document as the default "parent" element to attach the event listeners (forcing the user to provide an element "lower down" on the DOM might be too annoying); this means events will be triggered very (VERY) often, and having to check the target element against a large list of "potential" tooltip anchor elements on each trigger might end up having pretty much the same memory impact/performance than just attaching the event listeners to each anchor.

But I'm just guessing, this needs to be investigated. Anyway, thanks for the suggestion!


As a sidenote, we also do have an "imperative mode", which allows to control the tooltip programmatically without the need of event listeners necessarily. But depending on the use case, this method might be unviable. Just something to keep in mind.

@gabrieljablonski gabrieljablonski added the Awaiting merge Issue is fixed on a PR that will me merged soon. label Aug 1, 2024
@gabrieljablonski
Copy link
Member

Available on v5.28.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting merge Issue is fixed on a PR that will me merged soon. Feature Request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants