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

Android Auto: Set icon color when entity is considered in an active state #3805

Merged
merged 5 commits into from
Aug 24, 2023

Conversation

dshokouhi
Copy link
Member

Summary

Implements: https://community.home-assistant.io/t/add-accent-color-to-the-android-auto-ui/604634

Sets the icon color tint to the frontend yellow color if the entity is considered to be active according to the frontend

Also updates the Wear OS yellow color to more closely match the color from the HA frontend.

Screenshots

image

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#

Any other notes

I had thought about mimicking the frontend using blue for off and yellow for on but the screen looked a bit busy so decided to only change teh default color if the entity is considered active.

image

@jpelgrom
Copy link
Member

(also implements this review comment by a frontend member on the CarPlay PR, so design approved I"d say!)

Can you add a link to the frontend code for determining active state in a comment for that function, like the icon function has? If it is ever changed that makes it easier to reference and update.

@dshokouhi
Copy link
Member Author

(also implements this review comment by a frontend member on the CarPlay PR, so design approved I"d say!)

Can you add a link to the frontend code for determining active state in a comment for that function, like the icon function has? If it is ever changed that makes it easier to reference and update.

Sure once my home Internet is back up, storm took it down lol

@dshokouhi
Copy link
Member Author

Can you add a link to the frontend code for determining active state in a comment for that function, like the icon function has? If it is ever changed that makes it easier to reference and update.

finally back online, this is done

@jpelgrom
Copy link
Member

jpelgrom commented Aug 22, 2023

Looks like a lawn_mower domain was added yesterday which requires a custom case in the active states check...

Code looks good, but I'm seeing some differences that I'm not yet following. For example an entities card with 'Color icons based on state' with buttons is all blue in the frontend (= inactive), but all yellow in the Auto interface (= active).

Edit: think I found it right after I spent 15 minutes looking ;) https://github.com/home-assistant/frontend/blob/dev/src/common/entity/state_color.ts appears to be based on a list of domains as well whether or not the icon gets a color for active states. I would say add the check from lines 109-111, that seems easy enough but prevents making this function too complicated.

@dshokouhi
Copy link
Member Author

Looks like a lawn_mower domain was added yesterday which requires a custom case in the active states check...

lol were gonna have to be on top of newly added domains then with updating icons and availability in features. Probably a good idea to come up with a process for adding support for new domains

Code looks good, but I'm seeing some differences that I'm not yet following. For example an entities card with 'Color icons based on state' with buttons is all blue in the frontend (= inactive), but all yellow in the Auto interface (= active).

Edit: think I found it right after I spent 15 minutes looking ;) https://github.com/home-assistant/frontend/blob/dev/src/common/entity/state_color.ts appears to be based on a list of domains as well whether or not the icon gets a color for active states. I would say add the check from lines 109-111, that seems easy enough but prevents making this function too complicated.

Ah good find, went ahead and added this. Should hopefully take care of any inconsistencies :)

@dshokouhi
Copy link
Member Author

dshokouhi commented Aug 23, 2023

Not sure if we should do this here or in a future PR but noticed some of the colors are different.

Like cover is purple when considered active

https://github.com/home-assistant/frontend/blob/7c24e03125845f026220ddce3e524a98946ec030/src/resources/ha-style.ts#L126

Edit: actually not sure if we should add more colors just yet as we have to worry about contrast ratio 🤔 Maybe try this PR and if it passes review do a follow up adding the others?

@jpelgrom
Copy link
Member

I think the domains that can be colored list should be separate from isActive. Only used for coloring icons now but who knows what will be added in the future.

The yellow color that is used does appear to be the most common so let's stick to that for now. Not sure adding more colors later is a good idea - that's more logic to keep track of, and we never had complaints about Wear.

@dshokouhi
Copy link
Member Author

I think the domains that can be colored list should be separate from isActive. Only used for coloring icons now but who knows what will be added in the future.

Agreed, good call :) In fact we can probably apply isActive to tiles as we do something similar there

The yellow color that is used does appear to be the most common so let's stick to that for now. Not sure adding more colors later is a good idea - that's more logic to keep track of, and we never had complaints about Wear.

Thats a good point!

@jpelgrom
Copy link
Member

You forgot to update the MapVehicleScreen to check for state colored domain in addition to active :)

In fact we can probably apply isActive to tiles as we do something similar there

Good idea, nice to have it all in one place.

@dshokouhi
Copy link
Member Author

You forgot to update the MapVehicleScreen to check for state colored domain in addition to active :)

🙈

@JBassett JBassett merged commit 1bdbd3f into home-assistant:master Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants