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

Custom icon support #495

Closed
wants to merge 8 commits into from
Closed

Custom icon support #495

wants to merge 8 commits into from

Conversation

KNXBroker
Copy link
Contributor

This pull request adds ICON support for switches, lights, covers, sensors and binary sensors. Icons can be defined directly in the native YAML code, additional customizing is no more necessary. The icon attribute will overwrite any icon already defined by a device_class.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • I have performed a self-review of my own code
  • The documentation has been adjusted accordingly
  • The changes generate no new warnings
  • Tests have been added that prove the fix is effective or that the feature works
  • The changes are documented in the changelog
  • The Homeassistant plugin has been adjusted in case of new config options

@farmio
Copy link
Member

farmio commented Nov 16, 2020

I have an honest question: What is wrong about using customize: ? This adds a lot of code to maintain just to have the same feature configurable in a different spot.
Besides that, aren't binary_sensors icons defined via their device_class?

@marvin-w
Copy link
Member

I personally don't think we should allow icons in here. As @farmio mentioned you can just use customize: for it. This adds no real value to the library apart from unneccessarily blowing it up and adding more code we need to maintain. I'm sorry for your time and thank you for your work but maybe you should ask things like this beforehand in our discord :) - Don't feel bad!

@KNXBroker
Copy link
Contributor Author

Okay, a lot of other integrations provide this as a native feature. I personally prefer having all information at one place. Customizing is for me like a workaround, but I see, opinions differ.

@KNXBroker
Copy link
Contributor Author

Closed, feature shall not be a native part of the integration.

@KNXBroker KNXBroker closed this Nov 16, 2020
@KNXBroker KNXBroker deleted the custom_icon_support branch November 16, 2020 20:34
@marvin-w
Copy link
Member

I can have a look again when I do the new config scheme. Maybe we find a place there

@KNXBroker
Copy link
Contributor Author

KNXBroker commented Nov 17, 2020

@marvin-w , this would be great. I always want to have things as logic and simple as possible for end users. And - as a lot of other integrations offer this setting as a nativ feature - it's hard to me to understand as a newbie why I have to do the setting here for integration A and there for the integration B. Regards

@farmio
Copy link
Member

farmio commented Nov 17, 2020

We tend to see xknx as a stand-alone library for knx communication. Adding lots of HA specific string variables seems odd in this respect.

Once we get to use config-flow the customization should be HAs deal again, I think, so these variables wouldn't be needed anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants