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(cover): possibility to set a color per cover state with theme #384

Merged
merged 15 commits into from
May 1, 2022

Conversation

bramstroker
Copy link
Contributor

Description

For shutters, curtains etc. it's more logical to show the cover in an active state when it is closed. My first proposal was to introduce a toggle in the config to invert this logic.
In this PR I solved it by looking at the device class of the entity.
Also cleaned up the code a bit by introducing enums for the possible device classes and states of covers.

Related Issue

Fixes #227

Motivation and Context

See above

How Has This Been Tested

Tested with 3 different covers of different device classes (garage, shutter and window). When the shutter was closed the icon was in active state as expected, for the other devices the old logic still applies and did not see any regressions here.

Types of changes

  • 🐛 Bug fix (non-breaking change which fixes an issue)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 🌎 Translation (addition or update a translation)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have tested the change locally.

@acesyde
Copy link
Contributor

acesyde commented Apr 23, 2022

Hi @bramstroker

Instead of adding another option, you can add custom theme variables like --rgb-state-cover-{state} and let users customize colors :)

Exemple : Lock Card / Alarm card / Person Card / Update card

@piitaya
Copy link
Owner

piitaya commented Apr 23, 2022

Thanks for the PR 🙂

By default we will keep the colored state for open for all cover (shutters, garage, etc...) because it's defined like this in Apple Homekit and Google Home.
Also, Home Assistant (and Homekit and Google Home) set percentage to 100% when open so it's more logical to have colored state when percentage is not 0.

Example in Google Home (sorry for the french Screenshot 😅):
Screenshot_20220423-125746.jpg

I agree with @acesyde may be we can add theme variables for open and closed state color like the lock card to allow user to change the default behaviour.

@bramstroker
Copy link
Contributor Author

The Google Home approach seems also counter intuitive to me, but I understand you want to streamline this.

The problem with this theme variable is this would apply to the whole dashboard, so when I set another color for --rgb-state-cover-open in the theme it applies to all types of covers, but I don't want this behaviour for my window only for the shutters. So this will not really be a solution for me. But maybe I'm misunderstanding.

Maybe we can introduce a configuration for the cover card active_states or something like that, where the user can check/uncheck the states which are considered active (open, opening, closed, closing). open and opening would be the defaults. What do you guys think?

@bramstroker
Copy link
Contributor Author

@piitaya @acesyde Could you please have another look? I introduced a bunch of theme variables for the different states (as suggested) and removed the deviceclass logic I added.

I can now change the colors on a per card base with card_mod plugin.

type: custom:mushroom-cover-card
entity: cover.garage_door
card_mod:
  style: |
    :host {
      --rgb-state-cover-closed: var(--rgb-red);
      --rgb-state-cover-closing: var(--rgb-red);
      --rgb-state-cover-open: var(--rgb-grey);
      --rgb-state-cover-opening: var(--rgb-grey);
    }

It's not perfect, especially for not so tech savy people. And you'll need an additional plugin to do this. But for me it's workable this way.

@piitaya
Copy link
Owner

piitaya commented Apr 24, 2022

Thanks for the changes.
For the slider, how it works with your solution? Is it always grey ? Does it have to be inverted too?

@bramstroker
Copy link
Contributor Author

Thanks for the changes. For the slider, how it works with your solution? Is it always grey ? Does it have to be inverted too?

Good question. I did not use the position control myself currently so did not check.
But it stays grey with the current solution.
I think we should apply the same colors to the position slider. i.e. when you have set --rgb-state-cover-open to red and the cover is in open state the slider is also shown red.
What do you think?

@piitaya
Copy link
Owner

piitaya commented Apr 24, 2022

Thanks for the changes. For the slider, how it works with your solution? Is it always grey ? Does it have to be inverted too?

Good question. I did not use the position control myself currently so did not check.
But it stays grey with the current solution.
I think we should apply the same colors to the position slider. i.e. when you have set --rgb-state-cover-open to red and the cover is in open state the slider is also shown red.
What do you think?

Yep, but the logic of the slider is not inverted. You are using grey for open color so the slider will be grey.

@bramstroker
Copy link
Contributor Author

Please see my latest commit, which will also apply the state color variables to the slider control.

Which will give the result as shown below. The first card is the default behaviour, and the second one I inverted the open/closing states with card_mod.

Screenshot 2022-04-24 at 12 59 22

@piitaya
Copy link
Owner

piitaya commented Apr 24, 2022

I need some time to study the real issue. I think it's more a UX issue between open and closed state than just a color issue.
How can you have "closed" state with 43% ? I think I'm missing something about the cover behavior in HA.

@bramstroker
Copy link
Contributor Author

bramstroker commented Apr 24, 2022

The issue is about devices which are covering the window. For example a roller shutter. So my garage door in this screenshot is actually a bad example.
In normal state the roller shutter is up (open) and not covering the screen. In this case I want to show it inactive (grey).
However when the sun is shining I want to activate the roller shutter by rolling it down closed state (thus covering the window). This is when I want the roller shutter to show as activated (blue).
I can even roll the shutter down 50%, than it would be a legitimate case that the cover is shown as 50% closed.

I want to see in a glance which of my devices are in an active state. The roller shutters are now shown active all the time which is very confusing.

This PR solves this case, but also allow to define other colors than the default blue.

@piitaya
Copy link
Owner

piitaya commented Apr 28, 2022

Hi ! I'm coming back to you with a good news 🙂

I merged a PR to add disabled color. So you can now use :

type: custom:mushroom-cover-card
entity: cover.garage_door
card_mod:
  style: |
    :host {
      --rgb-state-cover-closed: var(--rgb-red);
      --rgb-state-cover-closing: var(--rgb-red);
      --rgb-state-cover-open: var(--rgb-disabled);
      --rgb-state-cover-opening: var(--rgb-disabled);
    }

I also have 2 comments :

  • the icon functions and all the stuff in ha folder are copied from Home Assistant front-end project. I think it can be better to let string variables for now for easier update if Home Assistant changes the rules. May be we can propose that to the official HA front-end ! 🚀
  • do you think we need a different color for closing state ? Maybe only open close colors can be enough ?

@Jn115759
Copy link

Hi both, I am not that deep in HA yet, not using styles. With the option to set color/icon available on the template card lot of flexibility is available for end users. Would it be an option to implement the same templating flexibility in the cover card ? This would allow to select icon as well as color per state of any entity. Or am I missing anything ?

@bramstroker
Copy link
Contributor Author

Hi ! I'm coming back to you with a good news 🙂

I merged a PR to add disabled color. So you can now use :

type: custom:mushroom-cover-card
entity: cover.garage_door
card_mod:
  style: |
    :host {
      --rgb-state-cover-closed: var(--rgb-red);
      --rgb-state-cover-closing: var(--rgb-red);
      --rgb-state-cover-open: var(--rgb-disabled);
      --rgb-state-cover-opening: var(--rgb-disabled);
    }

That's great. Will be able to do some development on this PR tomorrow.

I also have 2 comments :

  • the icon functions and all the stuff in ha folder are copied from Home Assistant front-end project. I think it can be better to let string variables for now for easier update if Home Assistant changes the rules. May be we can propose that to the official HA front-end ! 🚀

Ok, I will revert this changes. What's the reason why this logic is all copied from HA base, can't we just import these components from the core HA frontend somehow, which keeps the code DRY and bug fixing can happen upstream.

  • do you think we need a different color for closing state ? Maybe only open close colors can be enough ?

I think open and close will be enough. I don't see a reason why someone would prefer another color opening and closing. Will make this changes.

@bramstroker
Copy link
Contributor Author

Hi both, I am not that deep in HA yet, not using styles. With the option to set color/icon available on the template card lot of flexibility is available for end users. Would it be an option to implement the same templating flexibility in the cover card ? This would allow to select icon as well as color per state of any entity. Or am I missing anything ?

I would also like an icon selection and color picker for the open and close state in the GUI, which also makes things easier for less tech savy people and you don't need to have card_mod installed.
but I am not sure if @piitaya wants too many configuration options. If he also allows it to be added I can add that to this PR.

@Jn115759
Copy link

@bramstroker Thank you. I do understand things need to stay manageable but I think this would bring real added value

@bramstroker
Copy link
Contributor Author

Could be something like this. @piitaya what do you think?

Screenshot 2022-04-29 at 19 02 44

@piitaya
Copy link
Owner

piitaya commented Apr 29, 2022

I do not prefer to propose icon depending of state. Just choose the correct device_class to have the right icon. I prefer to keep mushroom simple. 99% of the user don't need that. Some entities like alarm panel and media player have multiple state and I don't want to add 4-5 icons to the form.

@piitaya
Copy link
Owner

piitaya commented Apr 29, 2022

Hi ! I'm coming back to you with a good news 🙂

I merged a PR to add disabled color. So you can now use :

type: custom:mushroom-cover-card
entity: cover.garage_door
card_mod:
  style: |
    :host {
      --rgb-state-cover-closed: var(--rgb-red);
      --rgb-state-cover-closing: var(--rgb-red);
      --rgb-state-cover-open: var(--rgb-disabled);
      --rgb-state-cover-opening: var(--rgb-disabled);
    }

That's great. Will be able to do some development on this PR tomorrow.

I also have 2 comments :

  • the icon functions and all the stuff in ha folder are copied from Home Assistant front-end project. I think it can be better to let string variables for now for easier update if Home Assistant changes the rules. May be we can propose that to the official HA front-end ! 🚀

Ok, I will revert this changes. What's the reason why this logic is all copied from HA base, can't we just import these components from the core HA frontend somehow, which keeps the code DRY and bug fixing can happen upstream.

  • do you think we need a different color for closing state ? Maybe only open close colors can be enough ?

I think open and close will be enough. I don't see a reason why someone would prefer another color opening and closing. Will make this changes.

HA front-end doesn't propose to import code so we need to copy all the rules 😅

@bramstroker
Copy link
Contributor Author

@piitaya Yes I agree, choosing icon per state would really be overkill and polute the GUI for an edge case.
But it is ok to add following fields? Than I will go ahead and implement this feature tomorrow.

  • Icon
  • Open color
  • Close color

@bramstroker
Copy link
Contributor Author

HA front-end doesn't propose to import code so we need to copy all the rules 😅

Ah, that's a pitty, maybe in the future..

@piitaya
Copy link
Owner

piitaya commented Apr 29, 2022

@piitaya Yes I agree, choosing icon per state would really be overkill and polute the GUI for an edge case.
But it is ok to add following fields? Than I will go ahead and implement this feature tomorrow.

  • Icon
  • Open color
  • Close color

I plan to add a GUI to edit mushroom variables to avoid using card_mod for color editing per state. To have a better customization for advanced users like you but from GUI 🙂

@bramstroker
Copy link
Contributor Author

I had it already working btw ;-, so can include it in PR.
I think it is a nice addition to the editor for covers as it are only 2 states.
But it's your call.

Screenshot 2022-04-29 at 19 58 31

)

@piitaya
Copy link
Owner

piitaya commented Apr 29, 2022

I had it already working btw ;-, so can include it in PR.
I think it is a nice addition to the editor for covers as it are only 2 states.
But it's your call.

Screenshot 2022-04-29 at 19 58 31 )

Yep but I don't prefer for consistency between cards 😅

@bramstroker
Copy link
Contributor Author

No problem, learned a bit trying to implement it. will finish the PR without this feature.

@bramstroker
Copy link
Contributor Author

PR is ready for final review now.

@bramstroker bramstroker changed the title Make the icon active state dependant on the device class of the cover feat(cover) Possibility to set a color per cover state Apr 30, 2022
@piitaya piitaya self-requested a review April 30, 2022 21:01
Copy link
Owner

@piitaya piitaya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put some comments about the color rules.

I also have good news for you : I will add a theme option to allow users to override theme for each card directly from the UI and without card_mod #423

src/cards/cover-card/cover-card.ts Outdated Show resolved Hide resolved
src/utils/theme.ts Outdated Show resolved Hide resolved
@bramstroker
Copy link
Contributor Author

I put some comments about the color rules.

I also have good news for you : I will add a theme option to allow users to override theme for each card directly from the UI and without card_mod #423

That's awesome.

Your comments about the colors are resolved.

Also resolved the merge conflicts again, for #417 and #422
Now I had to use ha-card instead of :host with card_mod to get the styles correctly applied. Anyway this will get much easier with the new feature in #423.

@piitaya piitaya changed the title feat(cover) Possibility to set a color per cover state feat(cover): possibility to set a color per cover state with theme May 1, 2022
@piitaya piitaya merged commit 032e668 into piitaya:main May 1, 2022
@piitaya piitaya added the enhancement New feature or request label May 1, 2022
@bramstroker bramstroker deleted the fix/cover-icon-active-state branch May 1, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inverse cover active icon
4 participants