Add ability to set Zwave protection commandclass by turbokongen · Pull Request #1433 · home-assistant/frontend

turbokongen · 2018-07-09T19:25:20Z

Allows the user via zwave control panel to set values of protection commandclass.
This is needed in some cases to allow programming of some devices.
See home-assistant/core#11137 for reference.
Accompanying PR for backend: home-assistant/core#15390

balloob · 2018-07-09T22:13:35Z

src/panels/config/zwave/ha-config-zwave.js


          <!--Config card-->
-          <zwave-node-config hass="[[hass]]" nodes="[[nodes]]" selected-node="[[selectedNode]]" config="[[config]]"></zwave-node-config>
+          <zwave-node-config hass="[[hass]]" nodes="[[nodes]]" selected-node="[[selectedNode]]" config="[[config]]" protection="[[protection]]"></zwave-node-config>


Add some new lines please.these long lines are really unreadable

balloob · 2018-07-09T22:14:59Z

src/panels/config/zwave/zwave-node-config.js

        value: false,
      },
+
+      protectionNode: {


All these protection options and html should be extracted in their own web component

balloob · 2018-07-09T22:15:48Z

src/panels/config/zwave/zwave-node-config.js


+  apiCalled(ev) {
+    if (ev.detail.success) {
+      var foo = this;


Foo...

Just use an arrow function and this will be set to defining context

balloob · 2018-07-09T22:18:14Z

src/panels/config/zwave/zwave-node-config.js

+
+  refreshProtection(selectedNode) {
+    var protectionValues = [];
+    this.hass.callApi('GET', 'zwave/protection/' + this.nodes[selectedNode].attributes.node_id).then(function (protections) {


We've been adopting async and await syntax.

async refreshProtection() { const protections = await this.hass.callApi(...); }

balloob · 2018-07-10T09:23:24Z

src/panels/config/zwave/ha-config-zwave.js


          <!--Config card-->
-          <zwave-node-config hass="[[hass]]" nodes="[[nodes]]" selected-node="[[selectedNode]]" config="[[config]]"></zwave-node-config>
+          <zwave-node-config hass="[[hass]]" nodes="[[nodes]]" selected-node="[[selectedNode]]"


Style like this:

<zwave-node-config hass="[[hass]]" nodes="[[nodes]]" ></zwave-node-config>

balloob · 2018-07-10T09:24:37Z

src/panels/config/zwave/zwave-node-protection.js

+
+    </style>
+      <div class="content">
+        <template is="dom-if" if="[[protectionNode]]">


This is weird, it would make more sense that this complete web component doesn't get rendered if no protection node. So add the if statement to the component that renders this component.

balloob · 2018-07-10T11:22:18Z

src/panels/config/zwave/ha-config-zwave.js

      this.notifyPath('hasNodeUserCodes');
    });
+    this.protectionNode = false;
+    this.notifyPath('protectionNode');


This should not be needed since protectionNode is a property.

balloob · 2018-07-10T11:23:17Z

src/panels/config/zwave/ha-config-zwave.js

+        },
+      },
+
+      protectionNode: {


Please prefix anything that is internal (not set by a component rendering this component) with an _. So make it _protectionNode

balloob · 2018-07-10T11:38:53Z

src/panels/config/zwave/zwave-node-protection.js

+        value: false,
+      },
+
+      protectionValueID: {


This and all properties below it are internal and should be prefixed with an _

balloob · 2018-07-12T08:28:15Z

src/panels/config/zwave/ha-config-zwave.js

        value: 0,
      },
+
+      protection: {


You're setting protection so it's an internval variable, name should be prefixed with _

balloob · 2018-07-12T08:29:27Z

src/panels/config/zwave/zwave-node-protection.js

+  }
+
+  async refreshProtection(selectedNode) {
+    var protectionValues = [];


Don't use var. Either use const or let.

balloob · 2018-07-12T08:30:12Z

src/panels/config/zwave/zwave-node-protection.js

+  async refreshProtection(selectedNode) {
+    var protectionValues = [];
+    const resp = await this.hass.callApi('GET', 'zwave/protection/' + this.nodes[selectedNode].attributes.node_id);
+    var protections = resp;


? why not assign directly to this?

balloob · 2018-07-13T11:12:23Z

ok to merge when backend PR has been merged

c727 · 2018-07-13T11:32:19Z

src/panels/config/zwave/ha-config-zwave.js

+
+      _protection: {
+        type: Array,
+        value: function () {


c727 · 2018-07-13T11:33:12Z

src/panels/config/zwave/ha-config-zwave.js

+    this.hass.callApi('GET', 'zwave/protection/' + this.nodes[selectedNode].attributes.node_id).then((protections) => {
+      this.protection = this._objToArray(protections);
+      if (this.protection) {
+        if (this.protection[0] === undefined) {


protection.length === 0

c727 · 2018-07-13T11:33:54Z

src/panels/config/zwave/zwave-node-protection.js

+
+  static get properties() {
+    return {
+      hass: {


hass: Object

c727 · 2018-07-13T11:34:28Z

src/panels/config/zwave/zwave-node-protection.js

+        value: { },
+      },
+
+      _nodePath: {


short syntax

c727 · 2018-07-13T11:34:33Z

src/panels/config/zwave/zwave-node-protection.js

+        observer: 'computeProtectionData',
+      },
+
+      _protectionOptions: {


short syntax

c727 · 2018-07-13T11:38:26Z

src/panels/config/zwave/zwave-node-protection.js

+  }
+}
+
+customElements.define('zwave-node-protection', ZwaveProtectionConfig);


use same name

c727 · 2018-07-13T11:39:09Z

src/panels/config/zwave/zwave-node-protection.js

+      _selectedProtectionParameter: {
+        type: Number,
+        value: -1,
+        observer: 'computeProtectionData',


'_computeProtectionData'

c727 · 2018-07-13T11:41:17Z

src/panels/config/zwave/zwave-node-protection.js

+      selectedNode: {
+        type: Number,
+        value: -1,
+        observer: 'nodesChanged'


use "complex" observer

static get observers() { return [ '_nodesChanged(nodes, selectedNode)' ]; }

c727 · 2018-07-13T11:42:02Z

src/panels/config/zwave/zwave-node-protection.js

+      <div class="content">
+        <paper-card heading="Node protection">
+          <div class="device-picker">
+          <paper-dropdown-menu label="Protection" dynamic-align="" class="flex" placeholder="{{_loadedProtectionValue}}">


dynamic-align class="flex"

c727 · 2018-07-13T11:42:39Z

src/panels/config/zwave/zwave-node-protection.js

+      .device-picker {
+        @apply --layout-horizontal;
+        @apply --layout-center-center;
+        padding-left: 24px;


padding: 0 24px 24px 24px;

c727 · 2018-07-13T15:51:30Z

src/panels/config/zwave/ha-config-zwave.js

+            hass="[[hass]]"
+            nodes="[[nodes]]"
+            selected-node="[[selectedNode]]"
+            _protection="[[_protection]]"


should be protection="[[_protection]]" then

So, ok. There seems to be different views on this. You say it's not going to have and underscore, and balloob says it to have underscore.
My opinion is without underscore, because we pass it up to zwave-node-protection card for further processing, thus not only an internal property.

Correct - If anproperty is public/ passed in, we don't use the underscore.

c727 · 2018-07-13T15:52:38Z

src/panels/config/zwave/zwave-node-protection.js

+    return {
+      hass: Object,
+
+      nodes: {


nodes: Array,

c727 · 2018-07-13T15:54:55Z

src/panels/config/zwave/zwave-node-protection.js

+    if (!this.nodes) return;
+    if (this._protection) {
+      if (this._protection.length === 0) { return; }
+      this.setProperties({ protectionNode: true });


you can set multiple at once

this.setProperties({ protectionNode: true, _protectionOptions: this._protection[0].value, _loadedProtectionValue: this._protection[1].value, _protectionValueID: this._protection[2].value });

homeassistant added the cla-signed label Jul 9, 2018

ghost added the in progress label Jul 9, 2018

turbokongen mentioned this pull request Jul 9, 2018

Add ability to set Zwave protection commandclass home-assistant/core#15390

Merged

3 tasks

balloob requested changes Jul 9, 2018

View reviewed changes

homeassistant added the cla-signed label Jul 10, 2018

balloob reviewed Jul 10, 2018

View reviewed changes

balloob reviewed Jul 12, 2018

View reviewed changes

Add protection selector

a4e20da

turbokongen force-pushed the zwave-protection branch from 34f545c to a4e20da Compare July 12, 2018 13:39

homeassistant added the cla-signed label Jul 12, 2018

balloob approved these changes Jul 13, 2018

View reviewed changes

c727 reviewed Jul 13, 2018

View reviewed changes

turbokongen added 3 commits July 13, 2018 15:45

Review changes

51ee426

Naming

967260e

forgot one literal

f37103a

c727 reviewed Jul 13, 2018

View reviewed changes

Further reveiw changes

002c843

balloob merged commit 0aee48c into home-assistant:master Jul 23, 2018

ghost removed the in progress label Jul 23, 2018

github-actions bot locked and limited conversation to collaborators Jul 7, 2022

Conversation

turbokongen commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob commented Jul 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

turbokongen commented Jul 9, 2018 •

edited

Loading

balloob Jul 10, 2018 •

edited

Loading