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

Show light's color temperature state when selected in more info #24254

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

gregpaton08
Copy link

Proposed change

When viewing a light's color temperature in the more info view it would only display brightness percentage. To see the color temperature you would have to move the slider which then changes the color temperate. This change simply displays the color temperature value in kelvin when the color temperature view is selected.

These screenshots show the before and after of the change.

Screenshot 2025-02-15 at 8 49 58 AM Screenshot 2025-02-15 at 8 50 11 AM

I have not added any new tests, but if you would like me to just let me know, thanks!

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

As long as you have a light configured that supports color temperate it should be easy to test.

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @gregpaton08

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@gregpaton08 gregpaton08 marked this pull request as ready for review February 15, 2025 14:14
@wendevlin
Copy link
Contributor

Cool addition thanks @gregpaton08!

Can you make it maybe the same as brightness. When I move the bar the header stays the old value until I am done sliding.

At your kelvin implementation the numbers jump around too often. Or maybe you can add a debounce for the header update?

@gregpaton08
Copy link
Author

@wendevlin thanks for taking a look!

Can you make it maybe the same as brightness. When I move the bar the header stays the old value until I am done sliding.

So it looks like this is rooted in a design difference between the brightness and color temperature slider components. The color temperate slider sets @slider-moved whereas the brightness slider does not.

https://github.com/home-assistant/frontend/blob/dev/src/dialogs/more-info/components/lights/light-color-temp-picker.ts#L80
https://github.com/home-assistant/frontend/blob/dev/src/state-control/light/ha-state-control-light-brightness.ts#L69

I would argue that the brightness slider should be updated to match how the color temperature slider works so that you can see the change in real-time, like how a physical slider would work. In any case, I think this is outside the scope of my proposed change, but I'd be happy to look at it and submit another PR.

At your kelvin implementation the numbers jump around too often. Or maybe you can add a debounce for the header update?

Looks like the bounce was already there. Here's a video from the dev branch without my changes. I can look into fixing this, but I think it should be in a separate PR.

Screen.Recording.2025-02-17.at.6.11.24.AM.mov

@piitaya piitaya added the Needs UX Pull requests requiring a review from the Home Assistant design team label Feb 19, 2025
@piitaya
Copy link
Member

piitaya commented Feb 19, 2025

Hi 👋 Thank you for the contribution.
I wanted to add more context about the current behavior.

The color temperature picker have this behavior because it's the UX as the color picker :

  • live update when moving the slider
  • the current value is not displayed because of this live update and because it doesn't really make sense for color (should we display the RGB code in the light is in color mode? I'm not sure).

I tagged UX on this PR as it requires UX team decision for that change because we already discussed that point few years ago when the light more info was redesigned.

Also, I agree, the cursor jump in the slider is a bug. I created a PR to fix this issue : #24312

@gregpaton08
Copy link
Author

@piitaya thanks for the info and the bug fix!

the current value is not displayed because of this live update and because it doesn't really make sense for color (should we display the RGB code in the light is in color mode? I'm not sure).

I think displaying the hex color code makes sense, and was actually planning to submit a PR for that change once this one was done 😄

I tagged UX on this PR as it requires UX team decision for that change because we already discussed that point few years ago when the light more info was redesigned.

Is this conversation somewhere on github or somewhere else that you can point me to? Just looking to get some more background on the design decisions. Thanks!

@KNXBroker
Copy link

Is it allowed to jump in here? I never understood why the current light temperature is not displayed, I would appreciate showing it. The current value is only shown if moving the slider, so I do not know the original once changed. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed Needs UX Pull requests requiring a review from the Home Assistant design team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants