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

docs: Add example for *Bar Chart with Highlighting on Hover and Selection on Click* #3485

Merged

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Jul 19, 2024

Fixes #3301

Utilises alt.when, introduced in #3427

image


@mattijn I thought adding this first would be something to refer to in the follow-up PR discussed in #3427 (comment)

Also revisiting since #2759 (comment) I have a better explanation for the empty=False placement.

For select, the Parameter is used twice - once with empty=False and another with the default.
Whereas highlight is used only once.

I think this could be helpful to add to the alt.condition(empty) and each .when(empty) doc, as this is a good justification for why you can pass empty there.

https://github.com/dangotbanned/altair/blob/ef81573b9c709989d3965f778a5b0c5d77f6d4c3/tests/examples_methods_syntax/interactive_bar_select_highlight.py#L26-L45

…tion on Click*

Utilises `alt.when`, introduced in vega#3427

Fixes vega#3301
Copy link
Contributor

@mattijn mattijn left a comment

Choose a reason for hiding this comment

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

Thanks! If you have more insight where to place empty=False, that would be great. I thought we could control everything within the alt.selection_point(), but that seems not to be the case.

I added two suggestions that shows on a single line the when().then()'s

@dangotbanned
Copy link
Member Author

Thanks! If you have more insight where to place empty=False, that would be great. I thought we could control everything within the alt.selection_point(), but that seems not to be the case.

I added two suggestions that shows on a single line the when().then()'s

Thanks for the review @mattijn

stroke_width = (
    alt.when(select).then(alt.value(2, empty=False))
    .when(highlight).then(alt.value(1))
    .otherwise(alt.value(0))
)

I've no strong feelings on the formatting (happy to change), but two points I wanted to mention:

  1. I simply ran:
>>> ruff format ./tests/*/interactive_bar_select_highlight.py

I did notice ruff seems to be disabled for the examples, so if this changed in the future - we'd end up back at the original.

  1. If a user copies the example, and is using ruff | black - they may find the result surprising/unfamiliar.

I would say generally though I'd be in favor of running all the examples through ruff, for greater consistency:

@mattijn
Copy link
Contributor

mattijn commented Jul 19, 2024

From a programming perspective I can understand that you just want to ruff everything.
But the reason why autoformatting is disabled on the examples is because the autoformatted syntax is often producing results of altair syntax that I would not advocate in writing as a user. If the user decide to autoformat the code once finished, that is fine by me. What do you think?

@dangotbanned
Copy link
Member Author

What do you think?

@mattijn all good with me, I'll accept the suggestions and we can get this merged

Co-authored-by: Mattijn van Hoek <[email protected]>
@dangotbanned
Copy link
Member Author

@dangotbanned
Copy link
Member Author

#3487 will fix the unrelated mypy issue

@mattijn mattijn merged commit 7f514c9 into vega:main Jul 20, 2024
11 checks passed
@mattijn
Copy link
Contributor

mattijn commented Jul 20, 2024

Thanks! Tests pass now🥳

@dangotbanned dangotbanned deleted the example-bar-w-highlight-hover-selection branch July 21, 2024 11:02
dangotbanned added a commit to dangotbanned/altair that referenced this pull request Jul 21, 2024
This was already being handled internally, but now aligns with `when` and provides a documentable parameter.
Related: vega#3485 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adapting Vega-Lite example "Bar Chart with Highlighting on Hover and Selection on Click"
3 participants