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

ZWave Panel Updates #1647

Merged
merged 7 commits into from
Sep 10, 2018
Merged

ZWave Panel Updates #1647

merged 7 commits into from
Sep 10, 2018

Conversation

cgarwood
Copy link
Member

@cgarwood cgarwood commented Sep 8, 2018

3 updates:

  • Added padding to the OZW log section so it's not right against the card border
  • The "Entities of this node" dropdown now shows the actual entity_id instead of a half-entity_id half-friendly variation.
  • Removed the "Node Information" card and replaced it with a "Node Information" button that opens the more-info popup for the zwave node. This allows for renaming from the zwave panel using the pre-existing cog in the more-info dialog, and gets rid of duplicate code, since the original Node Information card only showed the attributes of the zwave node, like the more-info card already does.

@ghost ghost added the in progress label Sep 8, 2018
@cgarwood
Copy link
Member Author

cgarwood commented Sep 8, 2018

Screenshot with the new Node Information button and entity_id in the "Entities of this node" dropdown:
image

@turbokongen
Copy link
Contributor

I like it! Very good change 👍

@@ -563,6 +557,11 @@ class HaConfigZwave extends LocalizeMixin(PolymerElement) {
};
}

nodeMoreInfo(ev) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you prefix private methods with _

@@ -563,6 +557,11 @@ class HaConfigZwave extends LocalizeMixin(PolymerElement) {
};
}

nodeMoreInfo(ev) {
ev.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this is necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally copied from the more-info code in dev-states, but tested it without this line and it looks like it still works, so I'll remove it.

@@ -563,6 +557,11 @@ class HaConfigZwave extends LocalizeMixin(PolymerElement) {
};
}

nodeMoreInfo(ev) {
ev.preventDefault();
this.fire('hass-more-info', { entityId: this.nodes[this.selectedNode].entity_id });
Copy link
Member

Choose a reason for hiding this comment

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

You'll need to include the EventsMixin to get this.fire

@cgarwood
Copy link
Member Author

cgarwood commented Sep 8, 2018

got a little commit-push happy with that last commit. got a couple things to fix still.

@cgarwood
Copy link
Member Author

cgarwood commented Sep 9, 2018

Should be good to review again.

@balloob balloob merged commit 94006a8 into home-assistant:master Sep 10, 2018
@ghost ghost removed the in progress label Sep 10, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Jul 7, 2022
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.

4 participants