-
Notifications
You must be signed in to change notification settings - Fork 793
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 4 missing carbon
themes, provide autocomplete
#3516
Conversation
And provide link to `vega-themes` playground
carbon
themescarbon
themes, provide autocomplete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! And great to have autocomplete on this :) Added one suggestion
"urbaninstitute", | ||
"vox", | ||
] | ||
|
||
VEGA_THEMES = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of defining the theme names twice, I think you could specify _ThemeName
as you've done here and then VEGA_THEMES
could become typing.get_args(_ThemeName
). See https://docs.python.org/3/library/typing.html#typing.get_args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @binste for the review!
I think I could rework this to use typing.get_args
, but wanted to note some things first that led to my duplication here.
I'm viewing _ThemeName
as a short-term solution:
- Explicitly marking it "private"
- Defining within a
TYPE_CHECKING
block to prevent it existing at runtime.
I don't think I've used this pattern anywhere in altair
before, but some examples in other projects (for their own reasons):
Examples
- https://github.com/pytest-dev/pytest/blob/dc756f4117f7cbabbe0866bab1fe562ac287cc03/src/_pytest/mark/structures.py#L428-L482
- https://github.com/pytest-dev/pytest/blob/dc756f4117f7cbabbe0866bab1fe562ac287cc03/src/_pytest/mark/structures.py#L501-508
- https://github.com/narwhals-dev/narwhals/blob/6fcc6a6ab3b3a5831db14d0e7e18fc45c43a0594/narwhals/schema.py#L13-L19
- https://github.com/narwhals-dev/narwhals/blob/6fcc6a6ab3b3a5831db14d0e7e18fc45c43a0594/narwhals/typing.py#L9-L32
IIRC, I'd need to move _ThemeName
out of that block or use typing.get_type_hints
I'd also need to remove "default", "opaque"
in the def of _ThemeName
or later during iteration:
Registration source
altair/altair/vegalite/v5/theme.py
Lines 45 to 61 in c12ca27
themes.register( | |
"default", | |
lambda: {"config": {"view": {"continuousWidth": 300, "continuousHeight": 300}}}, | |
) | |
themes.register( | |
"opaque", | |
lambda: { | |
"config": { | |
"background": "white", | |
"view": {"continuousWidth": 300, "continuousHeight": 300}, | |
} | |
}, | |
) | |
themes.register("none", dict) | |
for theme in VEGA_THEMES: | |
themes.register(theme, VegaTheme(theme)) |
For a long-term solution, I'd want to:
- Extract the names directly from source as (
VegaThemes: Literal[...]
) - Define
altair
themes as (AltairThemes: Literal["opaque", ...
)- Your comment which was what sparked this PR 😄
- changing-the-theme which states a plan to add more in the future
->_ThemeName
DefaultThemes: TypeAlias = AltairThemes | VegaThemes
- Annotate
themes.enable(name: DefaultThemes)
- Revisit the need for defining this across two
theme.py
modules, as part of Flatten the package structure #3337
Essentially I want to keep this as flexible as possible for now.
Planning to open another issue to discuss themes more generally.
There are other ideas I've had and I got the impression you might also have some of your own 🤝
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steps re long-term solution sound great! We could probably keep it simple and just fetch the themes from the latest source file in vega-themes which you linked. Or the file which was part of the latest release.
Let's continue the discussion in a new issue as you suggested, thanks for bringing it up! :) I have some notes on providing a better default theme and additional documentation around customization (advanced legend manipulation, vertical y-axis titles, y-axis on the right side with tick labels on top of the ticks, ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added 2 comments to mention that these two variables need to be kept in sync for now. Just in case we don't get to an improved solution for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steps re long-term solution sound great! We could probably keep it simple and just fetch the themes from the latest source file in vega-themes which you linked. Or the file which was part of the latest release.
Let's continue the discussion in a new issue as you suggested, thanks for bringing it up! :) I have some notes on providing a better default theme and additional documentation around customization (advanced legend manipulation, vertical y-axis titles, y-axis on the right side with tick labels on top of the ticks, ...)
Perfect, thanks again @binste
Related
Examples
Bonus
Support autocomplete for
themes.enable(name)
.