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

Improved rendering order of nodes/edges in Graph View #2623

Merged
merged 7 commits into from
Jun 22, 2017

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Jun 20, 2017

As a first step towards making #2431 work, we need to fix the rendering order of elements in <NodesChart />. The current order all edges -> all nodes doesn't look that bad as long as the background nodes are kept relatively transparent, but even so, on a closer look it can be seen how background nodes are rendered on top of foreground edges. This PR introduces a new rendering order:

  • blurred edges
  • blurred nodes
  • normal edges
  • normal nodes
  • highlighted edges
  • highlighted nodes
  • hovered edges
  • hovered nodes

Since SVG 1.1 doesn't support anything like CSS z-index, and SVG 2.0 standard still seems far away, the SVG rendering order necessarily corresponds to the DOM order, so I had to collapse both components <NodesChartNodes /> and <NodesChartEdges /> into <NodesChartElements /> to make the lists of nodes and edges intertwined.

No measurable performance loses were observed.

Now #2431 could be fixed e.g. by not making background nodes transparent, but changing their colors to similar effect.

@fbarl fbarl self-assigned this Jun 20, 2017
@fbarl fbarl requested a review from jpellizzari June 20, 2017 16:52
.map(this.edgeScaleDecorator)
.groupBy(this.edgeDisplayLayer);

// NOTE: The elements need to be arranged into a single array outside

This comment was marked as abuse.

This comment was marked as abuse.

Copy link
Contributor

@jpellizzari jpellizzari left a comment

Choose a reason for hiding this comment

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

Implementation LGTM

@fbarl
Copy link
Contributor Author

fbarl commented Jun 21, 2017

@jpellizzari I noticed later how the focused edges were blinking due to the CSS fading animation on them that was now being triggered on every hover with the changes I did. I added one more commit that fixes that by calculating edge thickness from <EdgeContainer /> and animating it through <Motion /> instead so PTAL at the behaviour before I merge it to make sure no new errors sneaked in.

@jpellizzari
Copy link
Contributor

@fbarl Looks reasonably smooth with no problems. LGTM.

@fbarl fbarl merged commit 36ee5db into master Jun 22, 2017
@fbarl fbarl deleted the proper-render-order-of-chart-nodes-edges branch July 7, 2017 18:07
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.

2 participants