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: Adds Vega-Altair Theme Test #3630

Merged
merged 40 commits into from
Oct 17, 2024
Merged

docs: Adds Vega-Altair Theme Test #3630

merged 40 commits into from
Oct 17, 2024

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Oct 3, 2024

Related: #3519 (comment)

Description

This PR provides a recreation of Vega Theme Test using altair, accessible via the User Guide.

The primary benefit is to provide theme authors some sample code that they can use for testing the effects of their theme config.
I mentioned this in #3519 (comment), but I really think it is valuable to have a quick way to experiment across a wide range of chart/mark types.

Additionally, it serves as an example of composing a complex compound chart.
Compared to the existing compound_charts section - this features a range of data sources w/ a mix of shared/unique and local/url.

Video preview

2024-10-17.16-52-37.-.Embedded.redesign.mp4

See #3630 (comment) for initial version w/ pyscript

Screenshots

Code block

Collapsed

image

Expanded

image

The addition of this new directive allows directly referencing the contents of a function in the docs.

There is also some interesting stuff in https://github.com/vega/altair/blob/3a5801edcbb0536b3031a32bc4984fdfe0d8fb8d/tools/codemod.py that can be adapted to support #3570 (comment).
The short explanation is this supports rewriting & reformatting code to be used elsewhere.
So this would have the benefit of writing in the same style as the rest of altair code, but support autoformatting for a possibly different style if agreed upon in #3519.

More generally though, a move towards writing code blocks in .py vs .rst has other benefits from a maintenance perspective:

  • Code reusability
  • Code blocks updating in response to IDE refactoring
  • Clearer diffs during review
  • Full access to tools like ruff, mypy to ensure code quality

Tasks

Deferred

  • Refactor/move tests.altair_theme_test.py (thread)
    • I've made a start on this with (6e1ca94)
    • Could do with a second opinion on where these kinds of source snippets should sit
    • Uses mkdocs, but polars/docs/source/src

Outdated

An earlier version that did not use pyscript:

Separate html page

Screenshots

Ignore the slightly yellow hue. Seems to be an artifact of the screenshot itself

fivethirtyeight

fivethirtyeight

latimes

latimes

vox

vox

default

default

Current plan is for this link to replace the current link to Vega Theme Test

See `Vega Theme Test`_ for an interactive demo of themes inherited from `Vega Themes`_.

Notes

Theme Selection (Planning)

My understanding (not a web dev) of how switching between themes works in Vega Theme Test:

**This doesn't work yet**.
The url params do work, but calling refresh doesn't
@dangotbanned
Copy link
Member Author

dangotbanned commented Oct 5, 2024

@joelostblom do you have any ideas on how I'd go about integrating this into the User Guide?

I'm not sure if https://github.com/vega/sphinxext-altair would make sense here?

Edit

Just spotted these that could be relevant

@dangotbanned
Copy link
Member Author

dangotbanned commented Oct 5, 2024

mypy issue in https://github.com/vega/altair/actions/runs/11194253679/job/31120512491?pr=3630 is unrelated to this PR

Fixed in #3632

dangotbanned added a commit that referenced this pull request Oct 5, 2024
# Related
- #3630 (comment)

```
tests\vegalite\v5\test_renderers.py:14: error: Incompatible types in assignment (expression has type "None", variable has type Module)  [assignment]
        vlc = None
              ^~~~
tests\vegalite\v5\test_api.py:32: error: Incompatible types in assignment (expression has type "None", variable has type Module)  [assignment]
        vlc = None
              ^~~~
tests\utils\test_mimebundle.py:10: error: Incompatible types in assignment (expression has type "None", variable has type Module)  [assignment]
        vlc = None
              ^~~~
tests\utils\test_compiler.py:10: error: Incompatible types in assignment (expression has type "None", variable has type Module)  [assignment]
        vlc = None
              ^~~~
Found 4 errors in 4 files (checked 332 source files)
```
@mattijn
Copy link
Contributor

mattijn commented Oct 14, 2024

It would be helpful to highlight the edge cases of a theme. My main concern is indeed the overall width and height in combination with the number of data points. By setting each chart to 160x160 and reducing the data entries per chart to a minimum, we can fit 6 charts (in a 2x3 grid) within the user guide's template.

@dangotbanned
Copy link
Member Author

It would be helpful to highlight the edge cases of a theme. My main concern is indeed the overall width and height in combination with the number of data points. By setting each chart to 160x160 and reducing the data entries per chart to a minimum, we can fit 6 charts (in a 2x3 grid) within the user guide's template.

@mattijn yeah that sounds like something we could try working towards.

I might need some help finding alternative examples that work better within these constraints.

Switching between these themes, you can see the varying height/width most clearly:

Also we have the theme definitions in altair now, which is handy for this task

Notes

Using the variable names in alt_theme_test, these are my initial observations:

  • geoshape, rect_heatmap
    • Probably the best candidates for limiting space
    • Both appear to cover roughly the same theme properties
    • Both are more niche mark types to use than the other examples
      • Maybe more so with geoshape, but for fields that need it I imagine it is vital
  • bar_facet
  • bar
    • Absolutely need to keep a basic bar example, since it is so common
    • Since it is not using real data, we could use a smaller slice of "Index" to reduce width
  • point, area
    • I've changed the dimensions of these quite a bit from the original
    • Currently, the axisX labels of urbaninstitute are already looking quite tight

@mattijn if you're available, any help towards this would be super appreciated.

Otherwise, I will loop back round to this part once I've finished off the work on the directive.
Got some quite funky results on a test I did yesterday, so I need to resolve a lot a of issues there

Bad generated html

Was attempting to create the dropdown using docutils.

Clearly misunderstood as these two look very different 😅

image

Fairly sure I can resolve this today though

Really unsure why `mypy` has suddenly started complaining about this
- Quick & dirty version
- Pushing to have something for regression testing while I finalize the directive api
@dangotbanned
Copy link
Member Author

dangotbanned commented Oct 14, 2024

Otherwise, I will loop back round to this part once I've finished off the work on the directive. Got some quite funky results on a test I did yesterday, so I need to resolve a lot a of issues there

feat(DRAFT): Adds Functional altair-pyscript directive

As of the above commit, I've now got a working version in the docs

2024-10-14.15-08-04.WIP.Functional.PyScript.mp4

Still needs some cleaning up of the code and fitting the dimensions - but @mattijn your suggestion to use pyscript was the right call, thank you!

Edit

See updated version in description

@mattijn
Copy link
Contributor

mattijn commented Oct 14, 2024

Awesome @dangotbanned! Really nice to see that this might be the first pyscript integration of an Altair chart within the documentation. And it actually works!

I can imagine we move elements of this into https://github.com/vega/sphinxext-altair.
Edit: in case you need it, I think you now have the rights to write to that repository too.

Reverts main part of 6e1ca94
Not needed now that `pyscript` is working
- Way less dependent on constants
- Similar api to `altair-code-ref`
- Leaving the door open for future potential `pyscript` use-cases
  - This is purely concerned with the theme switching
@dangotbanned dangotbanned marked this pull request as draft October 14, 2024 21:54
@joelostblom
Copy link
Contributor

As of the above commit, I've now got a working version in the docs

Both beautiful and really useful! 🙌

@dangotbanned
Copy link
Member Author

Awesome @dangotbanned! Really nice to see that this might be the first pyscript integration of an Altair chart within the documentation. And it actually works!

Edit: in case you need it, I think you now have the rights to write to that repository too.

Thanks x2 @mattijn

I can imagine we move elements of this into vega/sphinxext-altair.

Yeah I think some version of this would make sense there in the future.
I have left PyScriptDirective as a placeholder (at least) for now

class PyScriptDirective(SphinxDirective):
"""Placeholder for non-theme related directive."""

I'd probably need to find out some more use cases for pyscript vs altair-plot- and pair that with the right options to expose in a directive.
It looks like there is quite a lot more you can do within the <script> tag.


In comparison, ThemeDirective (note to self, needs renaming) uses pyscript but also some other things that are less generally useful:

  • Building up theme names from our annotations
  • Populating a dropdown menu (select element)
  • Referencing and rewriting code
    • python function -> embed within pyscript tag

These make a lot of sense here from a maintenance perspective (see Code Block in #3630 (comment)).
A more general case would probably want to support writing the code block inline (only or additionally), in a similar way to how vega/sphinxext-altair works via eval()

sphinxext/code_ref.py Outdated Show resolved Hide resolved
tools/codemod.py Outdated Show resolved Hide resolved
@dangotbanned dangotbanned marked this pull request as ready for review October 17, 2024 16:18
@dangotbanned dangotbanned requested review from mattijn and removed request for mattijn October 17, 2024 16:51
@dangotbanned
Copy link
Member Author

dangotbanned commented Oct 17, 2024

One thing I think might come up is the choice of legend(orient="bottom")

I moved the legend for area, bar_stack since they took up a lot of horizontal space with orient="right".

Initially tried "top", but that didn't play nicely with every theme

Screenshot

image

Edit

Oh thanks for approving already @mattijn 😄

@mattijn
Copy link
Contributor

mattijn commented Oct 17, 2024

Looks great 👍

@dangotbanned dangotbanned merged commit 0245e0f into main Oct 17, 2024
25 checks passed
@dangotbanned dangotbanned deleted the altair-theme-test branch October 17, 2024 17:20
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.

3 participants