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

Introducing Distinct Color Temperature Selectors: Mireds and Kelvin #18217

Closed

Conversation

schelv
Copy link
Contributor

@schelv schelv commented Oct 14, 2023

Proposed change

Described in home-assistant/core#101990 .

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

color_temp_mired:

color_temp_kelvin:

Additional information

Also described in home-assistant/core#101990 .

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:

@github-actions github-actions bot added the Design Related to Home Assistant design gallery label Oct 14, 2023
@schelv schelv force-pushed the new-color-temp-selector-attempt branch from 2ae922c to 89f3ef8 Compare October 14, 2023 11:00
@schelv schelv changed the title New color temp selector attempt Introducing Distinct Color Temperature Selectors: Mireds and Kelvin Oct 14, 2023
@schelv schelv marked this pull request as ready for review October 14, 2023 12:50
@bramkragten
Copy link
Member

bramkragten commented Oct 16, 2023

This would be a breaking change, because we remove an existing used selector.

Instead, we should add a mode parameter to the existing color temperature selector, this should default to mired, to not be breaking.

@schelv
Copy link
Contributor Author

schelv commented Oct 16, 2023

This would be a breaking change, because we remove an existing used selector.

The existing selector is still there!
So no breaking changes.
I did remove it from the documentation, and renamed the file and the class, to avoid further usage.

Instead, we should add a mode parameter to the existing color temperature selector, this should default to mired, to not be breaking.

This direction was something I considered.
But having min_mireds, max_mireds, min, and max configuration variables at the same time? 🤢
Combining mired and kelvin in one class also doesn't give code that is easy to read and understand, because of all the switch statements that have to be introduced.

@frenck
Copy link
Member

frenck commented Oct 16, 2023

But having min_mireds, max_mireds, min, and max configuration variables at the same time? 🤢

We can guard that. keeping min_mireds & max_mireds in backwards compatible. From the Core end, we can also handle that in the schema to migrate.

@schelv
Copy link
Contributor Author

schelv commented Oct 16, 2023

We can guard that. keeping min_mireds & max_mireds in backwards compatible.

I'm not quite sure what you mean by this.

From the Core end, we can also handle that in the schema to migrate.

I hadn't considered migration because I'm unsure how it would apply to blueprints. If migration is possible, does that not imply that maintaining backwards compatibility is unnecessary, since everything could be migrated?

@schelv
Copy link
Contributor Author

schelv commented Nov 11, 2023

New attempt to solve the problem in #18627

@schelv schelv closed this Nov 11, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed Design Related to Home Assistant design gallery hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants