-
-
Notifications
You must be signed in to change notification settings - Fork 179
place tooltip above line of line chart, otherwise fall back to default #128
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
base: main
Are you sure you want to change the base?
Conversation
…ttom position (default)
9a7047c to
da18084
Compare
|
@ramiy I rebased w/ latest |
|
@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. |
|
I remember looking to the playwright tests when I worked on this last year but couldn't get them to pass on If you have tips on how to get those running / passing, I am happy to take a look at adding more tests. |
|
@filmaj thank you for the PR. Interesting workaround. I'm also thinking to solve this with anchor-positioning. |
|
Pretty cool, I was not aware of this. If only that was more widely supported 😢 |
|
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. |
|
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 I'm currently using Mac, so locally it generates snapshot files with a different CI in GitHub Actions will run the tests on linux, changing the file suffix to |
|
Sounds good, thanks for the details For this PR, I will start by trying to add tooltip tests based on 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? |
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:
(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--startand--endvariables, and multiply it by100%, to change the tooltip position so that it always renders above the line: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 of0.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.