Fix visual map issues #20541
Fix visual map issues #20541
Conversation
- hover background color of zoom control in dark mode - map light mode when dark theme is used - background color of zoom control with map dark mode when light theme is used
- Additionally fixed unpleasant horizontal scrollbar in map editor
karwosts
changed the title
|
I did a review of the code and a test run, and it looks ready to be merged. The only thing that bothers me is that it doesn't actually "fix" the visual map issues right away. I would propose to have it a different title. If we're out to "fix" the issue we should have dark mode base with dark_mode: false to be light according to the comment in the linked issues. |
|
I had maybe thought we were trying to get away from the "ternary booleans", which was the point of migrating to the new variable, which does not have ambiguity between false and undefined. That's why I didn't add anything for |
|
We can also just make it an enum instead of a boolean; Or prepare to remove and deprecate this option and have it handled in themes completely 🙃 |
I believe that's what this has now. |
| @@ -170,6 +180,7 @@ class HuiMapCard extends LitElement implements LovelaceCard { | |||
| "ui.panel.lovelace.cards.map.reset_focus" | |||
| )} | |||
| .path=${mdiImageFilterCenterFocus} | |||
| style=${isDarkMode ? "color:#ffffff" : "color:#000000"} | |||
There was a problem hiding this comment.
This should be done with a class
| selector: { number: { mode: "box", min: 0 } }, | ||
| }, | ||
| { | ||
| name: "theme_mode", |
There was a problem hiding this comment.
Shouldn't we just leave this out of the UI and promote the use of the theme variable map-filter?
There was a problem hiding this comment.
If you say this option should be removed, and build it all with themes, I can understand that argument. I don't know if I fully agree with it, as I think that is pretty complicated to ask users to do, but I understand not wanting to maintain multiple solutions. In the past I have added theme vars to solve problems and I get a lot of "what's a theme? how do I add what where? help I broke my config", etc. So I think many are not too familiar with it, it's somewhat advanced. The map filter string is pretty intimidating as well.
If we did want to replicate this functionality with themes I think we at least need one new theme var, to set the color of the map icons, and the color of the Reset Focus button. If overall map background color is independently themable then those need to be independently themable as well to not clash.
So that is one decision to make.
However if we do decide to keep this theme_mode as an option, I'm not sure what the point is to build it, but then hide it by removing from the UI. Just seems kind of user-hostile for no reason.
Anyway let me know what you think so we can try to align on that and get this issue closed out. Thanks.
There was a problem hiding this comment.
I guess one of the issues I have is that once you set a theme_mode you can no longer theme the color of the controls, as they are hard coded then.
But I do agree that it is not easy to create a theme for things like this, but ideally a user using a community created theme doesnt have to change the config of cards to get it to match the theme.
Not sure what the right approach her would be.
Proposed change
Continuation of #17074 by @christophwen
To that PR added yaml migration, and fixed some of the behavior which broke when we changed darkmode from a ternary option to a strict boolean.
Light Mode Base:
Dark Mode Base:
Type of change
Example configuration
Additional information
dark_modewiththeme_modehome-assistant.io#32352Checklist
If user exposed functionality or configuration variables are added/changed: