-
Notifications
You must be signed in to change notification settings - Fork 136
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: support theming solara #494
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @iisakkirotko and the rest of your teammates on Graphite |
06c4657
to
de8a112
Compare
0599961
to
f8b0e84
Compare
de8a112
to
27b701b
Compare
b4d6684
to
10fe4e7
Compare
899a669
to
850c94b
Compare
f58e32e
to
d03f038
Compare
3cdce2b
to
1f4fc86
Compare
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 think this is going to be super cool!
Mostly worried about the naming, and maybe the theming.vue can be cleaned up a bit by using computed properties instead of using the clicks directly (difficult to follow).
(e.g. using a .is_auto() { return ...} )
3cea2ab
to
fcea18f
Compare
f981cfd
to
a9f836f
Compare
f9b54bf
to
84883d7
Compare
c41779e
to
162cf58
Compare
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.
Looking good.
I think we should remove totally the variant_user_selectable
and change
class ThemeSettings(BaseSettings):
variant: ThemeVariant = ThemeVariant.light
to
class ThemeSettings(BaseSettings):
dark: ThemeVariant = ThemeVariant.light
To make it backward compatible, we should check if SOLARA_THEME_VARIANT
is set, and make theme.dark reflect that.
Also, think about how this plays with the cmd line arg "--theme-variant
. Not sure how we should expose it with the rename, --dark=dark
seems a bit odd.
This could be a reason to keep the name variant, let me know what you think
5456d37
to
82e45dc
Compare
Co-authored-by: Maarten Breddels <[email protected]>
Co-authored-by: Maarten Breddels <[email protected]>
82e45dc
to
60015d9
Compare
No description provided.