Skip to content

Conversation

@filmaj
Copy link

@filmaj filmaj commented May 2, 2024

Thanks for the great library!

I was trying to think of ways of positioning the tooltip in a more convenient place when in line charts, as the default positioning is always 50%; depending on the actual chart, this may not be ideal:

Screenshot 2024-05-01 at 21 53 16

(the line chart/tooltip interaction in Firefox also has some rendering problems, as you can see chart segments to the right of the tooltip render "over" the tooltip, but that is a separate issue!)

In this PR, I suggest to use calc() to calculate the maximum of --start and --end variables, and multiply it by 100%, to change the tooltip position so that it always renders above the line:

Screenshot 2024-05-01 at 21 52 59

It might need a bit more padding, as you can see the "tail" of the tooltip is just slightly below the line segment still, but, I believe it is an improvement.

Also, using var()'s second parameter, the fallback parameter, we can fall back to using a value of 0.5 * 100% = 50%, which is the default (current) value today. In this way, I don't think this change should affect chart types other than line charts.

@filmaj filmaj force-pushed the line-chart-tooltip branch from 9a7047c to da18084 Compare July 21, 2025 15:22
@filmaj
Copy link
Author

filmaj commented Jul 21, 2025

@ramiy I rebased w/ latest main since your 1.2 release; perhaps you could reconsider this PR?

@filmaj filmaj requested a review from ramiy July 21, 2025 15:54
@ramiy
Copy link
Collaborator

ramiy commented Jul 21, 2025

@filmaj Thank you for the PR. Interesting approach.

I don't have Playwright tests for tooltips. I'll have to write additional tests to monitor visual regressions.

@filmaj
Copy link
Author

filmaj commented Jul 21, 2025

I remember looking to the playwright tests when I worked on this last year but couldn't get them to pass on main :/

If you have tips on how to get those running / passing, I am happy to take a look at adding more tests.

@ramiy
Copy link
Collaborator

ramiy commented Jul 21, 2025

@filmaj thank you for the PR. Interesting workaround.

I'm also thinking to solve this with anchor-positioning.

@filmaj
Copy link
Author

filmaj commented Jul 21, 2025

Pretty cool, I was not aware of this. If only that was more widely supported 😢

@filmaj
Copy link
Author

filmaj commented Jul 22, 2025

I think I'm getting closer to having the tests passing locally - it seems I need to figure out a local little static server myself, and generate image snapshots for tests to compare against. Does that sound correct?

Let me know if there are any other details - I can document them in the README, and I can also try setting up some CI in GitHub Actions so that these tests run on every pull request and report results.

@ramiy
Copy link
Collaborator

ramiy commented Jul 22, 2025

You are correct, the tests generate snapshots to compare against.

I think you are struggling because the snapshots originally generate on a Windows computer. That is why all snapshots have a -win32.jpeg suffix, added by Playwright.

I'm currently using Mac, so locally it generates snapshot files with a different -darwin.jpeg suffix.

CI in GitHub Actions will run the tests on linux, changing the file suffix to -linux.jpeg. It's a good change, but should be created in a separate PR.

@filmaj
Copy link
Author

filmaj commented Jul 23, 2025

Sounds good, thanks for the details

For this PR, I will start by trying to add tooltip tests based on main, probably in a separate PR. This way I can get comfortable w/ the tests first. Eventually I'll rebase this PR / update the tooltip tests in here. Then, I think, we can revisit this PR.

If you are up for discussing how to test this project in CI, perhaps I'll file an issue and we can discuss approaches/strategies/ideas in there?

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.

2 participants