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

Allow saving the light current color as a favorite #17992

Merged

Conversation

karwosts
Copy link
Contributor

@karwosts karwosts commented Sep 22, 2023

Proposed change

Allow to save the current light's color as a favorite, without having to select a new color.

This was actually a bit trickier than I thought it would be, I feel like I'm probably missing a more elegant solution, but I couldn't find it.

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

Additional information

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:

color_mode === LightColorMode.COLOR_TEMP &&
attributes.color_temp_kelvin
) {
this._color = {
Copy link
Member

@bramkragten bramkragten Sep 25, 2023

Choose a reason for hiding this comment

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

We should also set the correct LightPickerMode in this._mode

Copy link
Member

Choose a reason for hiding this comment

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

Mmmm I guess _updateModes does that, I think that logic should be refactored a bit so it makes more sense (combined)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I get what you're asking for. You want to fold updateModes and loadCurrentColor into a single function?

I don't know if I see much refactoring opportunity there, there doesn't seem like too much overlap between them.

Copy link
Member

Choose a reason for hiding this comment

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

The part that sets the _mode, I found it confusing that the _color and _mode are set at different places, as they are related, and both determined by the LightColorMode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I tried an alternate implementation. I'm not sure if I feel this is much better or less confusing, but it does set _color and _mode more consistently.

Copy link
Member

Choose a reason for hiding this comment

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

not really I think 😅 now we are calculating the current color always and only use it when add == true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I do only want the current color when creating a new preset, because otherwise the current color of the bulb is not what we want when editing an existing preset.

Let me know what's the path forward.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, no I get that :-) I'll propose something tomorrow, it is really nitpicking anyway....

Copy link
Member

@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 was thinking at another approach : passing the current color when editing a favorite and default to _loadCurrentColorAndMode if no favorite color is passed to the dialog (in case of adding a new color).
Also, this would means we don't have to pass the defaultMode anymore. But I agree with @bramkragten it's nitpicking. I'm ok to merge this one to fix the issue and we can refactor to improve code quality after if needed.

@bramkragten bramkragten merged commit acb32ae into home-assistant:dev Sep 26, 2023
8 checks passed
@karwosts karwosts deleted the light-favorite-color-picker-current branch September 26, 2023 17:01
@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Creating a custom color for the more-info color selectors doesn't work unless you select a color.
3 participants