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

feat: Adds @register_theme decorator #3526

Merged
merged 20 commits into from
Sep 5, 2024

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Aug 7, 2024

Important

Prior to v5.5.0, (#3618) refactored this into alt.theme.register.
See updated examples in https://altair-viz.github.io/user_guide/customization.html#defining-a-custom-theme

Resolves one item in #3519

Fairly simple addition, but it is a nice convenience when you only use a single theme.

I also added an example that might be relevant to @joelostblom #3519 (comment)

The decorator also works well with type checkers:

image

Adds `@register_theme` to top-level
@dangotbanned dangotbanned mentioned this pull request Aug 7, 2024
@dangotbanned
Copy link
Member Author

dangotbanned commented Aug 8, 2024

Will probably need to resolve some conflicts after merging

Edit

All good to go again

@dangotbanned dangotbanned linked an issue Aug 13, 2024 that may be closed by this pull request
Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

Very useful! Thanks for adding this and sorry that you had to wait a month for a review and keep this branch in sync ;)

Only one small question regarding LiteralString, otherwise this looks good to me.

altair/vegalite/v5/theme.py Show resolved Hide resolved
@dangotbanned
Copy link
Member Author

Very useful! Thanks for adding this and sorry that you had to wait a month for a review and keep this branch in sync ;)

Thanks for getting to this @binste and no worries on the timing; this is a mini-feature after all 😄

@dangotbanned dangotbanned requested a review from binste September 4, 2024 12:53
@dangotbanned dangotbanned enabled auto-merge (squash) September 4, 2024 17:40
Copy link
Contributor

@binste binste left a comment

Choose a reason for hiding this comment

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

Good point, having autocompletion trumps type hint flexibility in this case, wasn't aware that we can't have both. Thanks for the explanation!

Would you mind just adding the link to your GitHub comment as a Python comment next to the LiteralString type hints in register_theme and in the ThemeRegistry? Else, I'll probably change the type hint in a few months when I've forgotten why LiteralString is better :)

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