Fix issues related to panning to a node #824
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When a user selects a node that is out of view, the graph explorer pans to it and centers it. Unfortunately, this feature had some issues. 0. Panning happened before graph expansion completed. This led to pan target calculations that relied on incorrect, outdated measurements. We fix by waiting 250ms (the default duration for animations in the graph explorer) before panning in order to give enough time for node expansion to complete. 1. The graph explorer did not take into account the position of the graph's root SVG element with respect to the viewport. This led to panning calculations that were slightly off. We fix by taking that into account. 2. Nodes could sometimes be occluded by the minimap - those nodes are strictly speaking still in the view of the graph explorer, so we previously did not pan to nodes occluded by the minimap. Now, we do. There are still issues related to panning that are not fixed. For instance, we do not pan to extracted nodes. Panning sometimes yields a console error: ```js Error: <text> attribute x: Expected length, "NaN". ``` However, this PR does fix several big issues.
caisq
reviewed
Dec 18, 2017
There was a problem hiding this comment.
Thanks for this, @chihuahua! This is definitely improvement from before. But I am still seeing some buggy behavior, probably what this PR isn't intended to fix. I think we can check this in first (after addressing the other comment) and tackle the remaining issues later.
| isOutsideOfBounds(pointTL.y, pointBR.y, svgRect.height)) { | ||
|
|
||
| // Subtract to make sure that the node is not hidden behind the minimap. | ||
| const horizontalBound = svgRect.left + svgRect.width - 320; |
There was a problem hiding this comment.
Is there a way to avoid these magic numbers?
There was a problem hiding this comment.
I moved the numbers to constants.
chihuahua
commented
Dec 18, 2017
| isOutsideOfBounds(pointTL.y, pointBR.y, svgRect.height)) { | ||
|
|
||
| // Subtract to make sure that the node is not hidden behind the minimap. | ||
| const horizontalBound = svgRect.left + svgRect.width - 320; |
There was a problem hiding this comment.
I moved the numbers to constants.
caisq
approved these changes
Dec 18, 2017
|
Yeah ... we currently do not pan to const nodes. They appear attached as annotations to other nodes, and there could be several of them. Like this:
That will be fixed in a separate PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a user selects a node that is out of view, the graph explorer pans
to it and centers it. Unfortunately, this feature had some issues.
Panning happened before graph expansion completed. This led to pan
target calculations that relied on incorrect, outdated measurements. We
fix by waiting 250ms (the default duration for animations in the graph
explorer) before panning in order to give enough time for node expansion
to complete.
The graph explorer did not take into account the position of the
graph's root SVG element with respect to the viewport. This led to
panning calculations that were off by a constant amount along the X and Y
dimensions. We fix by taking those 2 values into account.
Nodes could sometimes be occluded by the minimap - those nodes are
strictly speaking still in the view of the graph explorer, so we
previously did not pan to nodes occluded by the minimap. Now, we do.
Screenshot:
There are still issues related to panning that are not fixed. For
instance, we do not pan to extracted nodes.
Also, Panning sometimes yields a console error:
However, this PR does fix several big issues.