-
Notifications
You must be signed in to change notification settings - Fork 59
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: improve theme auto-selection #296
feat: improve theme auto-selection #296
Conversation
Current dependencies on/for this PR: This stack of pull requests is managed by Graphite. |
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.
Nice!
js/src/Themes.js
Outdated
} else if (document.body.dataset.vscodeThemeKind === "vscode-dark") { | ||
// Special case for VS Code | ||
vuetify.framework.theme.dark = true; | ||
this.set("dark", true); | ||
this.save_changes(); | ||
} else if (document.body.dataset.vscodeThemeKind === "vscode-light") { | ||
vuetify.framework.theme.dark = false; | ||
this.set("dark", false); | ||
this.save_changes(); |
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.
So I think if dark==null signals 'auto', we should only set "dark_effective", and not "dark" right?
Also, should we listen to changes in the property?
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.
(Assuming we can listen to attribute changes)
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.
Ah, you're correct about it having to be dark_effective
.
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.
When you say
should we listen to changes in the property?
You mean dark_effective
?
c964eef
to
3610418
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 the code can be simpler with my suggestion and we still need to listen to changes in vuetify.framework.theme.dark
.
In the template we did sth like:
watch { 'vuetify.framework.theme.dark': (value) { ..} }
I'm not sure how we can do it here, maybe @mariobuikhuizen knows?
3610418
to
f803ab9
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.
Very nice, clean code 👍
vuetifyThemeChange() { | ||
if ( | ||
this.get("dark") !== null && | ||
this.get("dark") !== vuetify.framework.theme.dark | ||
) { | ||
this.set("dark", vuetify.framework.theme.dark); | ||
this.set("dark_effective", vuetify.framework.theme.dark); | ||
this.save_changes(); | ||
} |
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.
vuetifyThemeChange() { | |
if ( | |
this.get("dark") !== null && | |
this.get("dark") !== vuetify.framework.theme.dark | |
) { | |
this.set("dark", vuetify.framework.theme.dark); | |
this.set("dark_effective", vuetify.framework.theme.dark); | |
this.save_changes(); | |
} | |
vuetifyThemeChange() { | |
if ( | |
this.get("dark") !== null && | |
this.get("dark") !== vuetify.framework.theme.dark | |
) { | |
this.set("dark", vuetify.framework.theme.dark); | |
} | |
this.set("dark_effective", vuetify.framework.theme.dark); | |
this.save_changes(); |
I think we always want to set dark_effective right?
I think we can also spend a sentence on this feature in https://ipyvuetify.readthedocs.io/en/latest/usage.html#themes |
Original dark_jlab feature was added in #196 but was not yet released, so we don't have a breaking change. |
No description provided.