-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add support for custom themes to use dark mode #8347
Add support for custom themes to use dark mode #8347
Conversation
@@ -109,7 +109,7 @@ class LocationEditor extends LitElement { | |||
|
|||
if (changedProps.has("hass")) { | |||
const oldHass = changedProps.get("hass") as HomeAssistant | undefined; | |||
if (!oldHass || oldHass.themes?.darkMode === this.hass.themes?.darkMode) { |
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.
Why is this no longer needed? We still init themes
as null
?
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.
Should not have been needed before the PR either. Basically no usages of hass.themes
do check via ?
(e.g. none of the cards do).
if (key in STORAGE) { | ||
let value = JSON.parse(STORAGE[key]); | ||
// selectedTheme went from string to object on 20200718 | ||
if (key === "selectedTheme" && typeof value === "string") { | ||
value = { theme: value }; | ||
} | ||
// selectedTheme was renamed to selectedThemeSettings on 20210207 |
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.
Why did you rename this?
@@ -34,6 +34,7 @@ export const connectionMixin = <T extends Constructor<HassBaseEl>>( | |||
states: null as any, | |||
config: null as any, | |||
themes: null as any, | |||
selectedThemeSettings: null, |
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.
Shouldnt selectedTheme
be removed if we are going to rename it?
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.
selectedTheme
is not in this structure init, therefore cannot be removed here. Or am I missing your point?
… custom-dark-themes
… custom-dark-themes
Co-authored-by: Bram Kragten <mail@bramkragten.nl>
Co-authored-by: Bram Kragten <mail@bramkragten.nl>
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.
OK to merge when documentation is added
Breaking change
Not breaking, since legacy theme (= implicit light mode only) format is still supported.
Proposed change
Backend PR: home-assistant/core#46532
Adds the option for custom themes to be based upon the dark mode. A theme can now specify support for both light and dark mode and the user can select which one to use (same behavior as for the "default" theme mode selection).
This PR also renames the local storage parameter "selectedTheme" to "selectedThemeSettings" (to underscore that it does not only contain a theme name, but all related user defined settings) including an automatic migration for the old key storage.
This change also allows the user to specify default color values for light and dark mode forprimary_color
andaccent-color
. If provided, the same color pickers currently used by the "default" theme will be shown and users can adjust as desired.Reverted from MVP
Type of change
Example configuration
Below the different test themes used in the GIF above to go through the different scenarios.
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: