Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Extend
.interactive()
with tooltip and legend #3394base: main
Are you sure you want to change the base?
feat: Extend
.interactive()
with tooltip and legend #3394Changes from all commits
1e85a59
e76a3e2
7c127fd
d20ecc8
7fea185
343bb94
0b5b688
cbf51c4
29994be
8d85e02
f3a5a17
f2305bd
2981860
32f900e
cc3df47
706f374
3eb743e
4794028
a8001da
49bd736
b125c19
01644a2
24b8aab
708457c
51fd920
885767a
728d3cb
61281d7
5204759
ab7dab4
c6dac9d
5f47fa0
bd7ccfb
4b12df6
d9f8850
479fe27
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Same reasoning as for
a8001da
(#3394)Edit
Not exactly the same reasoning, as I think cases other than
"nominal"
are planned to be handled.Just unsure on which ones, so this could be updated with each
elif
case as they're completed?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.
Yes, that sounds good. I think the only other type we will support it
'ordinal'
. I don't see how either'quantitative'
or'temporal'
would make use of interactivity (I guess one could technically imagine an interval selection on a quantitative legend but that is currently not possible in VegaLite).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.
It wasn't clear to me what the intention was for this case.
At a minimum I think there should be a warning displayed, rather than silently continuing.
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'm not sure what is the best approach here. I agree that a warning/error is more explicit than silently continuing, but since
.interactive()
is a sort of convenient method to quickly add basic interactivity to charts, I also want to keep it simple to use. To me, the least friction for the user is if legend interactivity is applied when it makes sense and not applied when it doesn't make sense without being too noisy with warnings and errors. I'm thinking that people would not expect a gradient color legend to be interactive in any way, but they would expect to click on discrete points/lines to select and unselect groups. This is also how e.g. plotly works, see the examples on this page https://plotly.com/python/plotly-express/#gallery. If we go this path, one case that might be confusing in altair is quantitative scales in the size encoding since these appear to have discrete categories in the legend but clicking them would not select anything (since there are rarely observations with those exact values). I think this is minor, and am still leaning towards the "apply interactivity where it makes sense" approach for.interactive()
and I'm ok with this method being less explicitly verbose since I see it as more of a convenience.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 do see the argument for low-friction. The issue is the cost the solution has.
If I'm a new user and I call
Chart.interactive(legend=True)
, but result doesn't contain a legend, how am I supposed to know what went wrong?We have useful info in
.interactive()
that could quickly nudge the user, e.g. set one of the expected channels.The end result would be similar to the below example:
Screenshot from discussion
Without providing feedback like this, I'd assume we'd need to write the equivalent of that into the docstring anyway.
If that were the case, it would need repeating + maintaining in 7 places
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.
Good point, and it brings up an interesting distinction I didn't think of before. Whereas setting
tooltip=True
actually creates a tooltip, settinglegend=True
converts an existing legend into an interactive/clickable format (but doesn't create one from scratch as you pointed out). I will make this distinction explicit via the docstring and/or a more suitable parameter name. Then we can consider whether that is clear enough or we also need the warning/error; I appreciate your reasoning and suggestion with the nudging message, but I want to avoid that users need to passlegend=False
just because there color scale is using a quantitative encoding (so eventually I will likely favor either a warning or an explicit docstring, but I will leave the error raise for now until next time I can work on this PR)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.
That's fair!
My bad for writing the last comment without testing.
I was assuming it created a legend (forgetting that it would already be there).
I guess the starting point would be the same though, encountering that error currently would require
legend=True
and aChart
without a channel displaying a legend?