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

Fix inaccurate text size calculations #2505

Merged
merged 8 commits into from Oct 28, 2022
Merged

Conversation

EthanRutherford
Copy link
Contributor

Addresses #2475

As mentioned in the issue, the tooltip can appear to not have accurate sizing/padding in some cases.
I investigated and found that the tooltip tries to approximate the size that the text content will be, and then uses that to then size the tooltip box around the text. The logic for this size approximation lives here.

The logic for approximating the text size involves reading from a list of hard-coded font metadata, and computing styles and glyph widths to reach a final width and height. (victory-tooltip then adds the padding, and uses that for the size of the tooltip).
There's a few issues with this, the primary being that if a font is being used which is not in this list, the measurement could be off by an arbitrary amount. While that's not the case here, it is of course possible for a third party consumer to use their own font-set which is not in this list.

In this case, I discovered the culprit was the font-family list. It's common to list several fallback fonts for an element in css, but the logic here simply uses the first font family it finds in the list which it has metadata for. This may not match the font that is actually used during render, as the client may not have that font available. I am assuming that the reason this method doesn't make any attempt to query the browser for available fonts/measurements because this code may be called server-side, where it does not have access to the client it will be rendered on.

To remedy this issue, I've added logic to the tooltip component which measures the true size of the tooltip text using the DOM api, and does an immediate rerender with the accurate sizes. The rerender happens synchronously, before the browser does the next paint, and so the end user never sees the incorrect size.

before:
image

after:
image

Copy link
Member

@scottrippey scottrippey left a comment

Choose a reason for hiding this comment

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

A great approach, and I would approve, but there's actually a PR already open that does the same thing! #2352
Let's see if the PR above fixes the issue too?
This is a good reminder, too, that we should try to get one of these PRs merged!

@EthanRutherford
Copy link
Contributor Author

@scottrippey oh wow, whoops 😅 well, if there's a way I can help get one of these two merged let me know!

@EthanRutherford
Copy link
Contributor Author

@scottrippey I've updated the PR, bringing in the logic from the older PR and fixing some issues.

@ryan-roemer
Copy link
Member

Thanks for this work! Can we get a changeset added for this change too?

Co-authored-by: Ryan Roemer <ryan.roemer@formidable.com>
Copy link
Member

@scottrippey scottrippey left a comment

Choose a reason for hiding this comment

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

Nice work, looks great!

@EthanRutherford EthanRutherford changed the title Fix tooltip size not measuring accurately Fix inaccurate text size calculations Oct 25, 2022
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

3 participants