-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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(python): Add tooltip by default to charts #18625
Conversation
ab0ba24
to
0eb39b3
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18625 +/- ##
==========================================
- Coverage 79.94% 79.90% -0.05%
==========================================
Files 1505 1506 +1
Lines 202579 203219 +640
Branches 2873 2887 +14
==========================================
+ Hits 161958 162383 +425
- Misses 40073 40289 +216
+ Partials 548 547 -1 ☔ View full report in Codecov by Sentry. |
from altair.typing import ChannelColor as Color | ||
from altair.typing import ChannelOrder as Order | ||
from altair.typing import ChannelSize as Size | ||
from altair.typing import ChannelX as X | ||
from altair.typing import ChannelY as Y | ||
from altair.typing import EncodeKwds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just linking the context for this change #18609 (comment)
cc'ing @mattijn as this is a possible solution for vega/altair#3582 (comment)
tooltip | ||
Columns to show values of when hovering over bars with pointer. | ||
**kwargs | ||
Additional keyword arguments passed to Altair. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that this is a backwards-compatible change, as anyone passing tooltip
will still have their code work due to **kwargs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think tooltips by default is convenient and the approach here seems to make sense to me. Just one minor comment.
def _add_tooltip(chart: alt.Chart) -> alt.Chart: | ||
chart.mark = {"type": chart.mark, "tooltip": True} | ||
return chart |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are marks guaranteed to be strings here? It seems like they are by default in polars, but I'm flagging just in case you want to do something like this to check that the mark is not a dict (which you might already have seen).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelostblom I'm pretty sure @MarcoGorelli has been peeking at that PR 😉
Maybe I'm being overly cautious here, but I'm not sure we've tested this thoroughly enough for it to be included in polars
without any tests yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks both!
I've reworked this to create tooltip=
explicitly, reckon this is more robust / robust-enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks both!
I've reworked this to create
tooltip=
explicitly, reckon this is more robust / robust-enough?
Yeah this is great d15c672
(#18625)
Thanks @MarcoGorelli
Hi, this is causing issues on my side with Schema validation error: try using df.plot.line(x=alt.X("date", axis=alt.Axis(labelAngle=-90), y='price', color='stock') |
I think this is quite nice as the plot is then more interactive by default (and something Altair might want to do anyway? vega/altair#3394)
cc @joelostblom fancy taking a look? does this make sense?
Demo (the tooltip appears where I had my pointer):
Separate PR incoming to improve docs