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

Restructure and clarify interactive docs #2981

Merged
merged 27 commits into from
Mar 27, 2023
Merged

Restructure and clarify interactive docs #2981

merged 27 commits into from
Mar 27, 2023

Conversation

joelostblom
Copy link
Contributor

As I was adding the new interactive examples from #2760, I noticed several things I think we could express more clearly so I went through and overhauled this page. It is mostly clarification, and making titles a bit more succinct so that they are easier to get an overview from. I also restructured some sections mostly the widget one to try to give a more structured walk-through of the different options (I'm open to ideas if anyone thinks my suggestion could be improved).

Docs are live here so that it is easier to review https://joelostblom.github.io/altair-docs/user_guide/interactions.html#

Before:
image

After:
image

@joelostblom joelostblom requested a review from mattijn March 22, 2023 04:12
@joelostblom
Copy link
Contributor Author

I think I finally found a sensible way to organize the widget section in my last commit!

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.

Great improvements @joelostblom! I left a few comments in-line.


Interactivity in Altair is built around *parameters*, of which there are two types: *variables* and *selections*. We introduce these concepts through a series examples.
Parameters are the building blocks of interaction in Altair.
There are two types of parameters: *variables* (the :func:`param` function) and *selections* (the :func:`selection` function). We introduce these concepts through a series examples.
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels a bit strange to refer to selections since we just deprecate this function. Maybe better to not refer by a hyperlink? Or refer to selection_point, selection_interval. There is also a potential upcoming selection_region, so maybe not a good idea to mention all of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree and updated

doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
@@ -263,8 +240,49 @@ We can modify the brush definition, and leave the rest of the code unchanged:

chart.encode(x='Acceleration:Q') | chart.encode(x='Miles_per_Gallon:Q')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
chart.encode(x='Acceleration:Q') | chart.encode(x='Miles_per_Gallon:Q')
chart.encode(x='Horsepower:Q') | chart.encode(x='Displacement:Q')

I think we should keep Horsepower consistently on the x channel, in line with getting-started/overview page. And also start with the Miles_Per_Gallon/Horsepower on the left-chart and append another one on the right. I don't see much variation between the columns Accelaration and Miles_Per_Gallon, maybe better to use Displacement? (I've no feeling with the meaning of these column-names btw..)

If you agree, than we should apply this suggestion across all examples on this pages I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with this and updated the examples on the page

input_dropdown = alt.binding_radio(
# Add the empty selection which shows all when clicked
options=options + [None],
labels=labels + ['All'],
Copy link
Contributor

Choose a reason for hiding this comment

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

# Make radio button less cramped by adding a space after each label
options = ['Europe', 'Japan', 'USA']
labels = [option + ' ' for option in options]

I'm not sure if this spacing works. This is what I see as a result:
image

Copy link
Contributor Author

@joelostblom joelostblom Mar 26, 2023

Choose a reason for hiding this comment

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

Yes, the spacing does not work in the docs, but they do work in a jupyter notebook... I am meaning to fix the CSS in vegalite long term and then we can remove this. For now I added a note. ref vega/vega-lite#8799 (comment)

doc/user_guide/interactions.rst Show resolved Hide resolved
Comment on lines 632 to 635
For example,
we might want to create a condition
where we want to access the value of a slider
to use directly in a comparison with values of the data.
Copy link
Contributor

Choose a reason for hiding this comment

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

For example,
we might want to create a condition
where we want to access the value of a slider
to use directly in a comparison with values of the data.

It is a bit hard for me to understand this sentence, especially the last part. After reading it two times I get what you mean. Maybe split in two sentences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rephrased

doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
Copy link
Contributor Author

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

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

Thank you for the helpful comments! I have updated accordingly.

doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
input_dropdown = alt.binding_radio(
# Add the empty selection which shows all when clicked
options=options + [None],
labels=labels + ['All'],
Copy link
Contributor Author

@joelostblom joelostblom Mar 26, 2023

Choose a reason for hiding this comment

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

Yes, the spacing does not work in the docs, but they do work in a jupyter notebook... I am meaning to fix the CSS in vegalite long term and then we can remove this. For now I added a note. ref vega/vega-lite#8799 (comment)


Interactivity in Altair is built around *parameters*, of which there are two types: *variables* and *selections*. We introduce these concepts through a series examples.
Parameters are the building blocks of interaction in Altair.
There are two types of parameters: *variables* (the :func:`param` function) and *selections* (the :func:`selection` function). We introduce these concepts through a series examples.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree and updated

doc/user_guide/interactions.rst Show resolved Hide resolved
Comment on lines 632 to 635
For example,
we might want to create a condition
where we want to access the value of a slider
to use directly in a comparison with values of the data.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rephrased

@@ -263,8 +240,49 @@ We can modify the brush definition, and leave the rest of the code unchanged:

chart.encode(x='Acceleration:Q') | chart.encode(x='Miles_per_Gallon:Q')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with this and updated the examples on the page

@joelostblom joelostblom requested a review from mattijn March 26, 2023 19:46
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.

Few textual improvements. Looks good

doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
doc/user_guide/interactions.rst Outdated Show resolved Hide resolved
Co-authored-by: Mattijn van Hoek <mattijn@gmail.com>
@joelostblom
Copy link
Contributor Author

Great! I accepted all your suggestions. Merging.

@joelostblom joelostblom merged commit 37e91ec into master Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Documentation
Development

Successfully merging this pull request may close these issues.

2 participants