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

Node Collapse #83

Merged
merged 20 commits into from Jul 23, 2018

Conversation

@svipatov
Copy link
Contributor

svipatov commented Jun 29, 2018

@danielcaldas

When possible give it a look please.

  • Added a _toggleNodeConnection for toggling a node connection in this.state.links.
  • Added a _getLeafNodeConnections for finding leaf node connections based on a starting node id.
  • The toggles happen from 0 to 1 or 1 to 0 in the links matrix.
  • isHidden is added to the this.state.d3links.

Possible improvements:

  • Should we toggle also connections from the parent node -> leaf node?
  • isHidden being added as a default.
  • Separate file for node connections/links manipulations like the one mentioned above.

After a review I'll get started on testing and validating.

Copy link
Owner

danielcaldas left a comment

  • Should we toggle also connections from the parent node -> leaf node? I think we could leave this to a next iteration, still, I'm curious whether we could make this work with your current approach since you are preserving the matrix maybe we could apply something like the command pattern to undo/redo collapse interactions.

Nice job @svipatov! Thanks for putting your effort on this!

@@ -90,7 +90,19 @@ module.exports = {
},
{
source: 'VUX',
target: 'Intruder'
target: 'Intruder 1'

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Jul 1, 2018

Owner

It might be useful to have a specific data set (for instance collapsible) for this feature, this can be done by doing the following:

  1. Create a folder under sandbox/data/collapsible
  2. Add a config and a data file where you can tweak the data and configs in a more convenient way (collapsible.config.js and collapsible.data.js)
  3. Load the dataset in the sandbox by adding a query parameter to the url: ?data=collapsible

This comment has been minimized.

Copy link
@svipatov

svipatov Jul 1, 2018

Author Contributor

Sounds like an excelent idea! I can create another PR with this feature included without a doubt.

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Jul 2, 2018

Owner

Maybe this could be useful for this PR in order for us to do some functional tests on top of the dataset.

This comment has been minimized.

Copy link
@svipatov

svipatov Jul 3, 2018

Author Contributor

You are correct, haven't thought about the functional tests in this context.

@@ -358,11 +358,74 @@ export default class Graph extends React.Component {
this.pauseSimulation();
}

_toggleNodeConnection(targetNodeId, allConnections) {

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Jul 1, 2018

Owner

_toggleNodeConnection and _getLeafNodeConnections could be moved into graph.helper.js

This comment has been minimized.

Copy link
@svipatov

svipatov Jul 1, 2018

Author Contributor

Agree completely, at the time of the submission I decided to wait for your input where would be the best place for these functions :)

This comment has been minimized.

Copy link
@svipatov

svipatov Jul 1, 2018

Author Contributor

These have been moved! :)

}

onClickNode = clickedNodeId => {
const currentConnections = this.state.links;

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Jul 1, 2018

Owner

We could move all logic from line 387 to 411 in a separate function (maybe collapseNodes) the ouputs would be toggledLeafNodes and a new d3Links structure

This comment has been minimized.

Copy link
@svipatov

svipatov Jul 1, 2018

Author Contributor

We could, and we should.

I also attempted this separation, so in the future this could be done to any node.
But it may need a few tweaks to be a bit more resilient for other nodes (not just leaves).

This comment has been minimized.

Copy link
@svipatov

svipatov Jul 3, 2018

Author Contributor

I've moved this function for now into graph.helper.js. The discussion above could be a future concern :)

return Object.keys(nodes)
.filter(
nodeId =>
!!Object.values(linksMatrix[nodeId]).reduce(

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Jul 1, 2018

Owner

Maybe move this to a function countActiveNeighbours?

This comment has been minimized.

Copy link
@svipatov

svipatov Jul 1, 2018

Author Contributor

Sounds good!

This comment has been minimized.

Copy link
@svipatov

svipatov Jul 1, 2018

Author Contributor

I've created a getNodeCardinality in graph.helper.js.

Tell me what you think about the name. I felt that this way the name doesn't bind it directly to the concept of being active, but rather knowing how many nodes it has attached.

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Jul 2, 2018

Owner

But aren't we hiding the purpose of the function by detaching the name from knowing how many nodes it has attached?

This comment has been minimized.

Copy link
@svipatov

svipatov Jul 3, 2018

Author Contributor

We discussed this and the conclusion was that the name does make sense as being getNodeCardinality :)

Mostly so that it can be used in other contexts, and still making its responsibility transparent

@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Jul 2, 2018

A config should be added, to graph.config.js, below the steps to do it and documentation to add:

  1. In file graph.config.js https://github.com/danielcaldas/react-d3-graph/blob/master/src/components/graph/graph.config.js#L132 the config collapsible should be added, this has a default boolean value of false (note that all properties follow alphabetical order).
  2. Add the following piece of documentation to it:

@param {boolean} [collapsible=false] - 🚅🚅🚅 Allow leaf neighbours nodes to be collapsed (folded), this will allow users to clear the way out and focus on the parts of the graph that really matter. To see an example of this behaviour you can access this sandbox link that has a specific set up to experiment this feature.

  1. Apply collapse node logic conditionally in onClickNode by accessing this.state.config.collapsible
@svipatov svipatov force-pushed the svipatov:feature/node-collapse branch from 7ce030e to 93238fc Jul 3, 2018
@svipatov

This comment has been minimized.

Copy link
Contributor Author

svipatov commented Jul 19, 2018

Thanks for approving! I have not been able to add some functional tests yet. I'll try to find some time soon to do those!

@svipatov

This comment has been minimized.

Copy link
Contributor Author

svipatov commented Jul 20, 2018

@danielcaldas I've added a small functional test to verify the functionality :)

However I've had to change data.small.js, since the ID's were numerical, and since it is possible for us to use Object.keys, it would transform them into strings, mismatching ID's.

Solutions:

  1. Change data.small.js (Which was what I did).
  2. Try to parseInt ID's, if possible fine, if not, remain as a string.
  3. Use a Map to avoid this.
  4. Use == comparator so it coerces.

Tell me which one you think best?

(The culprit is in https://github.com/danielcaldas/react-d3-graph/pull/83/files#diff-f1693f8128c0a5fd1fd1ac84b9fd38cbR427)

@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Jul 20, 2018

The ids from data.small.js are numeric for that reason, to test a graph in an e2e scenario against numerical ids. From within rd3g we always parse those ids into strings (use something like leafNodeId => leafNodeId === '${source.id}' || leafNodeId === '${target.id}'), so I'd say you to choose option 2.. It's not ideal, but a possibility in the future is to drop the support for numerical ids and require clients to always use strings.

This has affected the component in the past, for instance here #73.

Nice job with the testing tough!!

@svipatov

This comment has been minimized.

Copy link
Contributor Author

svipatov commented Jul 20, 2018

I assumed it had a purpose correctly then. I'll implement your suggestion! Thank you :)

@svipatov

This comment has been minimized.

Copy link
Contributor Author

svipatov commented Jul 20, 2018

@danielcaldas danielcaldas merged commit df2e20f into danielcaldas:master Jul 23, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.