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

Focus view on a node #107

Merged
merged 30 commits into from Nov 13, 2018

Conversation

@LonelyPrincess
Copy link
Collaborator

LonelyPrincess commented Sep 10, 2018

The Graph component can receive a focusedNodeId prop with the id of an existing node. When that value is set, the node appear on the center of the visible part of the graph.

By default, the graph will be centered on the selected node, but the graph scale will not change. However, there's a new focusZoom property that can be added to the config object. If that property is set to a value different from 1, the selected zoom will be applied whenever the graph focuses on a node.

The sandbox example has been updated to preview this new feature. When the user clicks on a node, it will immediatly become the focused node.

It is now possible to set a "focusedNodeId" to the graph, which will
result in the selected node appearing on the center of the displayed
area of the graph.

Optionally, aside from centering the view on it, a certain zoom can be
applied while the node is focused on screen. No zoom is applied by
default, but that behaviour can be configured by using the new
"focusZoom" parameter in the config object. This setting should receive
a number with the zoom level to apply, so the larger it is, the bigger
the node will appear on screen.
@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Sep 22, 2018

Is this feature working properly already? I ran the sandbox in your branch and it seems that the desired effect it's not reproducible. Can you please check this? Thanks!

@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Sep 22, 2018

Thanks for checking it out, @danielcaldas !

I've just tried it again, and I can see the effect in the default sandbox example. When you click on a node, that one is selected as the focused one and the animation triggers. Since there are so many nodes there, it's easier to see if you set the focusZoom property to a higher value. By default, the node is centered on the graph view, but the size of the elements remains the same.

The current implementation seems to have some trouble in the small sample, though. 🙁

The problem is that the id of the node received by the onClickNode handler and the actual d3 node stored in the Graph component have different data types: one is a string, while the other is a number.

Since I was using === to search for the selected node and retrieve it's current position, the function in charge of centering the node is unable to find anything and no changes are made to the view.

In order to avoid those kind of data type issues, I'd probably have to convert both ids to string when performing the search. I'll try it and upload a patch right away. 👍

Nested ternary expressions replaced by a more readable if-else
expression when checking if the `focusZoom` property set by the user is
between the configured `minZoom` and `maxZoom`.
Copy link
Owner

danielcaldas left a comment

This actually works quite nicely when setting staticGraph to true 😉.

Take into consideration this comments for the near future, because before merging this there is a bug that needs to be fixed, regarding nodes animation reset, this is being triggered in moments that it shouldn't, I'll take a look at this.

Again thanks for the amazing contribution! 🎉

docs/DOCUMENTATION.md Show resolved Hide resolved
sandbox/Sandbox.jsx Outdated Show resolved Hide resolved
src/components/graph/Graph.jsx Outdated Show resolved Hide resolved
src/components/graph/Graph.jsx Outdated Show resolved Hide resolved
src/components/graph/Graph.jsx Outdated Show resolved Hide resolved
src/components/graph/Graph.jsx Outdated Show resolved Hide resolved
src/components/graph/graph.config.js Outdated Show resolved Hide resolved
src/components/graph/graph.helper.js Outdated Show resolved Hide resolved
Instead of being a prop on its own, the 'focusedNodeId' must now be
included as part of the 'data' object received by the Graph component.

The sandbox code has been update to reflect this change.
The 'getCenterAndZoomTransformation' has been rellocated to the graph
helper file, instead of being part of the Graph component.
@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Sep 30, 2018

Thanks a lot for your feedback, @danielcaldas ! I'm glad it's looking good! 😄

I've made all of the changes you requested me to, but don't hesitate to let me know if there's something else to be done.

Copy link
Owner

danielcaldas left a comment

Just commented on some small details

src/components/graph/Graph.jsx Outdated Show resolved Hide resolved
src/components/graph/graph.config.js Outdated Show resolved Hide resolved
src/components/graph/graph.helper.js Show resolved Hide resolved
src/components/graph/graph.helper.js Outdated Show resolved Hide resolved
@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Sep 30, 2018

No more comments to add, just check the snapshot test for the graph component ( Snapshot - Graph Component). You'll probably need to update the snapshot. And also a e2e test is failing:

[rd3g-graph] graph tests when clicking a node should collapse leaf nodes:
     CypressError: Timed out retrying: expected '<path>' not to be 'visible'

Please check whether collapsible nodes are working properly on your branch

@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Oct 1, 2018

Thanks! I tried and you are right: collapsible nodes doesn't seem to work in my branch. I've found out it fails after I add setState to the onClickNode function in the sandbox example. It works perfectly if I remove it, though. It also works when I add focusedNodeId to the editable graph data field.

I'll keep looking into it and upload a fix as soon as possible.

@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Oct 13, 2018

Hi there! Sorry for the delay, @danielcaldas ! It's gonna take a while longer to merge this PR in, as I haven't been able to fix this yet. 😞 I found out what the source of the problem is, though. Here's my report, so you can get an idea of what's the current status for this.

The reason why the collapsible nodes are not working as expected in the Sandbox example (and also in the tests) is that the graph data is been overriden when we call setState from the user defined onClickNode handler. This causes the Graph component to forget which of the edges and nodes are supposed to be hidden.

I suspected this may be related to the custom handler and the default onClickNode defined in Graph updating the state at the same time. Inside of the default onClickNode handler there is a call to a tick method which uses setState as well, and this is where trouble comes in.

I thought that ensuring both handlers run in order could help, so I tried using the setState callback parameter to ensure the custom onClickNode handler wasn't called until the Graph component had updated its inner data with the collapsed nodes / links:

this._tick({ ...disconnectedLeafNodesPartialState }, () => {
   this.props.onClickNode && this.props.onClickNode(clickedNodeId);
});

With this change, the links with the isHidden property did indeed remain hidden when the sandbox onClickNode updated its state.

The nodes which should be collapsed, however, did not disappear. The linkMatrix received by buildGraph in the render function was outdated, and since getNodeCardinality returned a value higher than zero even when one of its connections should no longer be there, the affected nodes weren't considered orphan and they were displayed on the screen.

Fixing this issue is proving to be kind of tricky, but I'll keep looking into it when I have the time. If something clicked to you while reading this, don't hesitate to let me know what you think. 👍

@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Oct 14, 2018

Thanks for the detailed report!

Indeed the call to _tick should be passed as a callback in the setState that is a thing that I've done in this branch feature/directional-graph while implementing directional graphs, I massively refactored the collapsible logic in order to support both directionals on directional graphs.

Maybe when I merge the branch you could update this one and check again whether the problem is going to be there at that time (my hope is that it should go away 🤞)

@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Oct 15, 2018

The directed branch was merged now, please give it another try! 😁

@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Oct 16, 2018

Good job, @danielcaldas ! Those directional links are looking nice! 👍

I merged the changes and tried again, but unfortunately it seems we're still facing issues here. 😢

I'll have to look further into it before giving you a more detailed report, but after trying things out in the Sandbox, this is the current situation:

✔️ The focused node is updated correctly everytime a node is clicked.

When a node is clicked for the first time, the links to collapse are hidden, but the orphan nodes are not.

When the same node is clicked again, the collapsed links don't reappear. Normally the collapse should be undone and all of those links should return to their original state (isHidden: false).

✔️ If a different node is clicked instead, collapse is applied to the new node and the previous one returns to its original un-collapsed state.

The link matrix was not been properly updated when the collapsible
setting was enabled. This is because when obtaining its values it didn't
check whether the different links were hidden or not.

This change fixes the issue and prevents the link matrix from containing
wrong values which cause orphan nodes are not hidden properly during
collapse.
@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Oct 16, 2018

I think I finally got it! 😄

The issue with the nodes not been hidden when they should was caused by the linkMatrix having the wrong values. The method in charge of obtaining this matrix, _initializeLinks, didn't check whether the links been added to the matrix were hidden or not.

Since isNodeVisible relies on this matrix to determine if a node should be displayed, no nodes would ever be hidden during collapse mode as long as data.links contained any link for them.

I modified the function _initializeLinks so it takes a link's isHidden property into account when it obtains its value, and everything seems to be working as expected now. Tests are not failing anymore either, which is also a good thing.

When you have the time to, could you try it out and confirm that the sandbox example works fine now? 🙏

PS: I submitted another PR fixing a small bug in your new branch. Merge that one in before trying this, because the custom onClickNode handler which applies the focus won't work otherwise.

Copy link
Owner

danielcaldas left a comment

x When a node is clicked for the first time, the links to collapse are hidden, but the orphan nodes are not.

x When the same node is clicked again, the collapsed links don't reappear. Normally the collapse should be undone and all of those links should return to their original state (isHidden: false).

Could not relicate those two (tried master and your branch on the commit has 4cc8ef62890c69f8c8917fa461ff52248ce1e278 where you merged with master) 🤔 Still the fix that you provided by looking into the isHidden property to create the linksMatrix makes total sense, thanks for looking into that.

Also overall the feature looks nice but there are two things that need our attention:

  • I could not collapsible and new zoom animation simultaniously
  • The animations are affecting overall graph zooming, as in it adds a certain delay when performing zoom on the graph probably due to the transform added by default? Is this something that you are aware of/experienced as well?

Rather than that I'm very happy with this new feature 😃

src/components/graph/graph.helper.js Show resolved Hide resolved
@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Oct 17, 2018

Could not relicate those two (tried master and your branch on the commit has 4cc8ef6 where you merged with master) 🤔

Oh, you're right! I cloned my branch and what I mentioned before do not happen to me either. There must have been something messed up in my local branch, though I can't really tell what it was. 😕

I'm experiencing both of these issues in my newly downloaded branch now, so I'll take a look and them as soon as possible. 👍

The animations are affecting overall graph zooming, as in it adds a certain delay when performing zoom on the graph probably due to the transform added by default? Is this something that you are aware of/experienced as well?

To be honest, I hadn't noticed it because I didn't try to make zoom normally. Thanks for letting me know!

That is due to the style prop I added in the containerProps. The css transition-duration property takes the value 0 by default, which is what is been applied in the current master branch. With the latest changes, however, the duration for that animation is customizable via the focusAnimationDuration setting.

We have two choices here:

  • Rename the focusAnimationDuration setting to make it more generic. This duration will be used to animate all of the transformations that trigger inside of the graph container, such as scale and translate (these are used to zoom and focus respectively).

  • Set the transition-duration property only when the value for focusNodeId has changed.

What do you think? How would you like it done? 🙂

@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Oct 17, 2018

I've just updated my remote branch by merging it with the current master in this repository. The zoom and focus animation was not working with the collapsible nodes because this commit hadn't been merged yet, so the onClickNode which actually set the focusedNodeId property was never been called.

This leaves us only with the transition duration issue, so I'll be awaiting your feedback before getting into that. 👍

@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Oct 17, 2018

Of course 🤦‍♂️ Without the callback fix that would never work.

Regarding the focusAnimationDuration if we could have this:

Set the transition-duration property only when the value for focusNodeId has changed.

It would be awesome. Another thing that I notice aside of this zooming issue is that the drag&drop functionality for the whole graph seems a bit off, could you take a look into that as well?

The transition duration property was been applied to the graph container
at all times, which affected to every other transformations that
triggered on it. This included the default zoom or the drag and drop
functionality, among others.

In order to fix this, that transition duration value is set back to zero
after the focus and zoom animation has finished. Some actions on the
node do also cancel the animation in case it's still ongoing, like
trying to drag a node.

With this change, there's no longer a delay when moving a node to
another location or while performing a zoom.
@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Oct 29, 2018

I know it's been a while, but I'm still not done with this. Sorry! 😢

The issue with the transition duration should be fixed by now. Whenever the animation finishes, its value resets to zero. With this change, the normal zoom should be done at its usual speed and not be affected by the focus animation duration.

Another thing that I notice aside of this zooming issue is that the drag&drop functionality for the whole graph seems a bit off, could you take a look into that as well?

I've been looking into this, and the translate transformation which is been applied to focus the graph on a certain node seems to be the source of the problem. You can perfectly drag nodes around when there's no focused node, but if there is one, moving a node has unexpected results. I'm still trying to come up with a solution for this issue. I'll let you know as soon as I make some progress.

The latest changes I submitted seem to have introduced some other errors in the Travis build, so I'll have to check that as well.

There was an issue with the drag and drop feature when used along the
focus and zoom transformation.

The problem was that we were recalculating the transformation everytime
the page was rendered, which meant that a new transform value would be
created anytime the node moved to a new position.

This issue only affected the focused node itself, since the
`getCenterAndZoomTransformation` did only depend on that node
coordinates.

To avoid abnormal behaviours, the transformation is only obtained when the graph receives props. By doing this, the current view position is preserved until a new focused node is set. If the user drags the node around, current zoom and position will be unaffected by it. This matches the default behaviour we encounter in any other scenario, when moving any other nodes or not having activated the focus and zoom feature.
The `enableFocusAnimation` property in Graph' state is now set to
`false` in the `dragStart` event instead of during `dragMove`. This
change should have a positive impact in the application's perfomance, as
the `enableFocusAnimation` value will only checked once instead of being
read in every position change during a drag operation.
@LonelyPrincess LonelyPrincess force-pushed the LonelyPrincess:feat/focus-on-node branch from 3c0bfff to 63c92b1 Oct 29, 2018
@LonelyPrincess LonelyPrincess force-pushed the LonelyPrincess:feat/focus-on-node branch from 67520f8 to aac9946 Oct 29, 2018
@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Oct 30, 2018

I finally put an end to the drag issue, and I also fixed the build errors that came up in recent commits. Let me know if you encounter any other issue and I'll take a look at it asap. 🙂

@LonelyPrincess LonelyPrincess self-assigned this Oct 30, 2018
@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Nov 8, 2018

Hey there @LonelyPrincess, strange thing happening to me in the sanbdox 🤔

When I click on a node in order to see the zooming animation the graph just disappears from the viewport. Have you been able to replicate this behaviour?

One of the latest changes broke the focus feature in certain scenarios.
When a focus zoom different from one was applied, the view was not
properly centered in the selected node. The transformation obtained in
the 'getCenterAndZoomTransformation' did not work as expected. Applying
a single translate before scaling didn't seem to do the trick, and the
second translate is necessary for the view to display the selected node
in the center after scaling the graph.
@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Nov 13, 2018

Hi there, @danielcaldas ! Sorry for not answering earlier, I had a pretty busy week and wasn't able to look into this.

I managed to replicate the behaviour you mentioned only when the focus zoom was set to a value different from zero. It seems I messed things up in my latest commit by removing one of the translate transformation functions in ' getCenterAndZoomTransformation'.

I changed it back to how it was before. After scaling the view, I apply the transformation to center the view on the selected node. Check it out when you have the time and let me know if it's working for you now. 👍

Copy link
Owner

danielcaldas left a comment

Nice work! Seems to work pretty good now, just left a few comments, please review those, they are minor details at this point, overall this is ready to be merged

src/components/graph/Graph.jsx Outdated Show resolved Hide resolved
Copy link
Owner

danielcaldas left a comment

Nice! ✔️

@danielcaldas danielcaldas merged commit 0374b09 into danielcaldas:master Nov 13, 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.