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

Add zwave_js device statistics #12794

Merged
merged 20 commits into from Jun 7, 2022
Merged

Conversation

raman325
Copy link
Contributor

@raman325 raman325 commented May 26, 2022

Proposed change

Adds a new zwave_js device action to show device statistics. Dependent on home-assistant/core#72520

The screenshot below doesn't show that I split protocol and data rate into two separate rows

Looks like this:
image

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

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:

@bramkragten
Copy link
Member

We should update the styling of the data, the text is too small now, and should be in primary color

@raman325
Copy link
Contributor Author

raman325 commented May 27, 2022

OK all comments addressed. I got feedback from Dominic that Not Available as indicated at the bottom of the screenshot above is confusing (it means the RSSI couldn't be measured but he thinks it looks like it implies the repeater device is not available). I've taken another approach to this which to leave the RSSI blank with an ha-help-tooltip there to indicate the error (there are multiple error scenarios). I am not particularly happy with it and would welcome other suggestions. Side note - any idea why the tooltip is so offset from the mouse? Perhaps because it's in the list item?

image

Comment on lines 416 to 422
Object.entries(this._deviceIDsToDevices).forEach(([_, device]) => {
if (!devices.includes(device))
delete this._deviceIDsToDevices[device.id];
});
devices.forEach((device) => {
this._deviceIDsToDevices[device.id] = device;
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Object.entries(this._deviceIDsToDevices).forEach(([_, device]) => {
if (!devices.includes(device))
delete this._deviceIDsToDevices[device.id];
});
devices.forEach((device) => {
this._deviceIDsToDevices[device.id] = device;
});
const devicesIdToName = {};
devices.forEach(device => {devicesIdToName[device.id] = computeDeviceName(device, this.hass)});
this._deviceIDsToName = devicesIdToName;

This makes sure it triggers an update when the devices are updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied the same approach to this._workingRoutes

bramkragten
bramkragten previously approved these changes May 30, 2022
raman325 and others added 2 commits May 30, 2022 17:50
@raman325 raman325 requested a review from bramkragten May 30, 2022 21:53
raman325 and others added 3 commits May 31, 2022 09:32
…log-zwave_js-node-statistics.ts

Co-authored-by: Bram Kragten <mail@bramkragten.nl>
…log-zwave_js-node-statistics.ts

Co-authored-by: Bram Kragten <mail@bramkragten.nl>
@bramkragten bramkragten merged commit 5437722 into home-assistant:dev Jun 7, 2022
@raman325 raman325 deleted the node_statistics branch June 7, 2022 14:49
@github-actions github-actions bot locked and limited conversation to collaborators Jun 8, 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.

None yet

3 participants