Bugs with focusNodeId #341
Conversation
1. When the user is passing hight/width props, the graph size isn't set to those parameters. 2. If we move the graph to a new position while it's focused on a specific node, the graph will jump to an unwanted place. 3. If we move the graph to a new position while it's focused on a specific node, and we will focus on the same node again, the graph will not change its position.
Once the user passed FocusNodeId to the graph, and he moved the graph to a new position, if componentWillReciveProps will call again the graph will always return to the location of the Focus Node. On the other hand, if the user cancels the focus node and passes an undefined parameter to the graph, the graph will return to the initial position and will be a jump in the position when we will render again the Graph component. We want the graph to always keep its last position - when there is a focus node and when there is no, and when we will render the component again (componentWillReciveProps), the graph will not change its position unless we passed focus node id.
There was a problem hiding this comment.
Thanks for the PR. I'll just request that you address the comments I left and additionally please fix the lint issues pointed in the CI failures. To check for lint errors locally use npm run lint.
I'll have another look and test once the comments are addressed. Thanks once more for you contribution.
src/components/graph/graph.helper.js
Outdated
| @@ -32,6 +32,9 @@ import ERRORS from "../../err"; | |||
|
|
|||
| import { isDeepEqual, isEmptyObject, merge, pick, antiPick, throwErr, logWarning } from "../../utils"; | |||
| import { computeNodeDegree } from "./collapse.helper"; | |||
| import { select as d3Select } from "d3-selection"; | |||
| import * as d3 from "d3"; | |||
| import { zoom as d3Zoom } from "d3-zoom"; | |||
There was a problem hiding this comment.
| import { zoom as d3Zoom } from "d3-zoom"; | |
| import { zoom as d3Zoom, zoomIdentity as d3ZoomIdentity } from "d3-zoom"; |
There was a problem hiding this comment.
Import from d3-zoom instead.
src/components/graph/graph.helper.js
Outdated
| @@ -32,6 +32,9 @@ import ERRORS from "../../err"; | |||
|
|
|||
| import { isDeepEqual, isEmptyObject, merge, pick, antiPick, throwErr, logWarning } from "../../utils"; | |||
| import { computeNodeDegree } from "./collapse.helper"; | |||
| import { select as d3Select } from "d3-selection"; | |||
| import * as d3 from "d3"; | |||
There was a problem hiding this comment.
| import * as d3 from "d3"; |
src/components/graph/graph.helper.js
Outdated
|
|
||
| const selector = d3Select(`#${containerElId}`); | ||
| // in order to initilize the new position | ||
| selector.call(d3Zoom().transform, d3.zoomIdentity.translate(transform.x, transform.y).scale(transform.k)); |
There was a problem hiding this comment.
I'll need to verify that focusAnimationDuration doesn't break with this change. @LonelyPrincess if you have some time could you please have a look at this one? I remember you implemented this one initially. Thanks!
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@veredwpsemantix feel free to bring this PR back to life 🙏 |
This PR includes fixing those bugs:
https://share.vidyard.com/watch/cEnBeTdYgZeYRnp4PxMMvL?
https://share.vidyard.com/watch/mzsVrDypdxVMsfosq5kV6x?