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

feat(ui): improve mesh graph #458

Merged
merged 27 commits into from
Feb 8, 2021
Merged

feat(ui): improve mesh graph #458

merged 27 commits into from
Feb 8, 2021

Conversation

robertsLando
Copy link
Member

@robertsLando robertsLando commented Feb 4, 2021

Fixes #384

Preview:

Schermata da 2021-02-05 16-27-45

@robertsLando robertsLando marked this pull request as draft February 4, 2021 16:30
@AlCalzone
Copy link
Member

Show us screenshots 🥸

@robertsLando
Copy link
Member Author

@AlCalzone nothing good to show yet, tomorrow you will see something 🙏🏻😎

@zwave-js zwave-js deleted a comment from zwave-js-assistant bot Feb 4, 2021
@robertsLando
Copy link
Member Author

Found the reason: https://observablehq.com/@d3/d3v6-migration-guide

// add nodes to graph
for (let i = 0; i < nodes.length; i++) {
const node = nodes[i]
g.setNode(node.id, node)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@robertsLando according to https://github.com/dagrejs/graphlib/wiki/API-Reference#setNode the second parameter to setNode() is the label, so I think it should be g.setNode(node.id, node.id) instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

nope, label can be an object so it's correct to use the node object there AFAIK. Did you try to use a string instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right. Yes, I've tried and the graph vanished ;-).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was just a misleading documentation to name it "label" if it can be an object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So maybe https://github.com/dagrejs/graphlib/wiki/API-Reference#setDefaultNodeLabel can be used to set the default label for nodes. I'll try.

@zwave-js-assistant
Copy link

🚧 It seems like this PR has lint errors 🚧
I should be able to fix them for you. If you want me to, just comment
@zwave-js-bot fix lint

@robertsLando robertsLando marked this pull request as ready for review February 5, 2021 15:56
@coveralls
Copy link

coveralls commented Feb 5, 2021

Pull Request Test Coverage Report for Build 547441464

  • 0 of 12 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 20.745%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/assets/convert.js 0 12 0.0%
Totals Coverage Status
Change from base Build 547440321: -0.02%
Covered Lines: 1996
Relevant Lines: 9861

💛 - Coveralls

@ahochsteger
Copy link
Collaborator

@robertsLando I'd like to keep you up-to-date about the approach I'll take on the filter possibilities for the mesh graph we talked about privately and get some feedback on it.

Since I'd like to reuse as much of the filtering code as possible from the nodes table on the control panel I'm going to refactor much of it into an UI-independent reusable component that provides functionality that can be used by the table as well as the mesh graph.
The UI-specific parts will be adjusted as well to be reusable on both sides.

I'm creating a ManagedItems component that takes an array of items (nodes), a property definition object and a string prefix for the local storage key names (e.g. nodes_) and encapsulates the following functionality:

  • Visible columns
  • Property filters
  • Grouping
  • Table page size
  • Item selection
  • Persistence of settings

It will have absolutely no hard-coded dependency on Z-Wave nodes, so it may be used as well for other item collections (e.g. hass devices, ...) if it makes sense.

This would also allow us to view the table and the mesh graph just as a different visual representation of the same things and keep the filters, selection, ... on both sides in sync (in case we'd want to have it that way) or keep them completely separated.

Of course everything will be quality checked using unit tests :-).

@robertsLando
Copy link
Member Author

I think that would be awesome! Nice idea @ahochsteger ! Btw I think we could merge this for now and keep that for another PR? At least we can get some feedbacks about the new implementation :)

@ahochsteger
Copy link
Collaborator

I think that would be awesome! Nice idea @ahochsteger ! Btw I think we could merge this for now and keep that for another PR? At least we can get some feedbacks about the new implementation :)

Yes, let's merge it and I'll start a new PR.

@robertsLando robertsLando merged commit 656e56d into master Feb 8, 2021
@robertsLando robertsLando deleted the improved-mesh-graph branch February 8, 2021 09:00
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.

[feat] Improved Z-Wave Network Visualization
4 participants