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

Documentation for each mark type #2607

Merged
merged 19 commits into from
Nov 4, 2022
Merged

Documentation for each mark type #2607

merged 19 commits into from
Nov 4, 2022

Conversation

hebarton5
Copy link

This PR includes subpages for each mark. All content is based off of the vega-lite mark documentation (https://vega.github.io/vega-lite/docs/mark.html), with adjustments made to make it applicable to Altair.

@mattijn
Copy link
Contributor

mattijn commented May 27, 2022

🎉👍

Copy link
Contributor

@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.

This is looking great @hebarton5! Thank you so much again for contributing to the Altair docs. And I am sorry for my slow review here, I was away for a few days and got busy at work when I returned. I added some comments on the index page and the first few mark pages. If you can reply to these and add the remaining marks to the page index, I will be able to review the rest.

========================================= ========================================= ================================================================================
:ref:`user-guide-arc-marks` :meth:`~Chart.mark_arc` A pie chart.
:ref:`user-guide-area-marks` :meth:`~Chart.mark_area` A filled area plot.
bar :meth:`~Chart.mark_bar` A bar plot.
Copy link
Contributor

Choose a reason for hiding this comment

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

I like linking the pages here as you have done for the arc and area mark. Could you do the same for the remaining marks?

:hidden:

arc
area
Copy link
Contributor

Choose a reason for hiding this comment

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

Include all the marks here as well so that they show up in the navigation menu. Ping me once you have done this and I will rebuilt the docs and review all the marks after "Area" as well.

Comment on lines +67 to +71
Area Config
^^^^^^^^^^^
The ``arc`` property of the top-level ``config`` object sets the default properties for all arc marks. If mark property encoding channels are specified for marks, these config values will be overridden.

The area config can contain any area mark properties (except ``type``, ``style``, and ``clip``).
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove this config section for each of the marks? I don't think it adds anything of value to the Altair side of things; it is more relevant for the VegaLite specs.

Arc marks are circular arcs defined by a center point plus angular and radial extents.
Arc marks are typically used for radial plots such as pie and donut charts.

Examples
Copy link
Contributor

Choose a reason for hiding this comment

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

Were you able to explore whether it is possible to add the "Properties" table for each mark page? Like in https://vega.github.io/vega-lite/docs/arc.html#properties and point 3 of #2578 (comment)

As seen in the last two examples, additional arguments to ``mark_*()`` methods are passed along to an
associated :class:`MarkDef` instance, which supports the following attributes:

.. altair-object-table:: altair.MarkDef
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to divide this table into subsections as in https://vega.github.io/vega-lite/docs/mark.html#mark-def? "General", "Color", etc.

Instead of using a single color as the fill color of the area, we can set it to a gradient.
In this example, we are also customizing the overlay. For more information about gradient options see the Vega-Lite Gradient documentation.

.. altair-plot.::
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an additional . before the :: that prevents this chart from showing up in the built docs

)

Instead of using a single color as the fill color of the area, we can set it to a gradient.
In this example, we are also customizing the overlay. For more information about gradient options see the Vega-Lite Gradient documentation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ranged Area
^^^^^^^^^^^
Specifying ``x2`` or ``y2`` for the quantitative axis of area marks produce ranged areas. For example, we can use ranged area with the ``ci0`` and ``ci0``
aggregation operators to highlight 95% confidence interval of a line chart that shows mean values over time.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you link to this page for the available aggregation operators? user_guide/encoding.html#binning-and-aggregation

^^^^^^^^^^^

We can also shift the stacked area chart’s baseline to center and produces a streamgraph by setting ``"stack"`` to ``"center"`` in the encoding channel.
Adding the ``interactive`` method allows for changing the scales.
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
Adding the ``interactive`` method allows for changing the scales.
Adding the ``interactive`` method allows for zooming and panning the x-scale.

@joelostblom
Copy link
Contributor

@hebarton5 Just checking in on this, would you be interested in working on addressing the comments I made?

@mattijn
Copy link
Contributor

mattijn commented Aug 16, 2022

Or merge and accept as being "infrastructure code" and iterate further? I think this has been work part of an (given?) assignment.

@joelostblom
Copy link
Contributor

Yeah, that makes sense, I won't have time to iterate on it for a while myself so maybe we should merge this when one of us have time to rewrite it so that these doc pages are not broken on the main branch?

@mattijn
Copy link
Contributor

mattijn commented Aug 17, 2022

Make sense, first fix conflicts before merging.

@binste
Copy link
Contributor

binste commented Nov 4, 2022

@joelostblom I probably find some time during the next 4-5 weeks to address the open comments in this PR. However, I don't think I can edit this PR directly right? Could you merge it first or is there a better approach?

@mattijn mattijn merged commit 8dd1990 into vega:master Nov 4, 2022
@mattijn
Copy link
Contributor

mattijn commented Nov 4, 2022

I fixed the conflict. @binste, from here you can create a new PR to work on the review from @joelostblom. Thank you for for offering help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants