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 4 missing carbon themes, provide autocomplete #3516

Merged
merged 6 commits into from
Aug 4, 2024

Conversation

@dangotbanned dangotbanned changed the title feat: Adds 4 missing carbon themes feat: Adds 4 missing carbon themes, provide autocomplete Aug 3, 2024
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.

Thanks! And great to have autocomplete on this :) Added one suggestion

"urbaninstitute",
"vox",
]

VEGA_THEMES = [
Copy link
Contributor

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

Copy link
Member Author

@dangotbanned dangotbanned Aug 4, 2024

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

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

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", ...)
  • _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 🤝

Copy link
Contributor

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, ...)

Copy link
Contributor

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.

Copy link
Member Author

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

@binste binste merged commit 15f1151 into vega:main Aug 4, 2024
13 checks passed
@dangotbanned dangotbanned mentioned this pull request Aug 4, 2024
@dangotbanned dangotbanned deleted the update-vega-themes branch August 4, 2024 17:43
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