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

Add data layers #129

Merged
merged 68 commits into from
Apr 7, 2020
Merged

Add data layers #129

merged 68 commits into from
Apr 7, 2020

Conversation

richardwestenra
Copy link
Contributor

@richardwestenra richardwestenra commented Mar 13, 2020

Description

  • Separate graph nodes vertically (by layer)
  • Add layer bands, and highlight them on hover

image

Development notes

This is a really big PR - sorry about that!

The layered rankings depends on being able to set ranker: "none", a feature in Dagre that currently only exists on a PR that has been open since June last year. To get this to work we'll either need to fork Dagre or see if we can get the PR resurrected and merged. Else, we'll need to switch to a new DAG layout engine or write our own. In the short term, I've mirrored the branch from the PR on my own GitHub account and linked to it in package.json, so that we at least have ownership of it and can be sure that it won't be deleted or altered.

Tasks

  • Prototype layer zoom/pan on background strips and label layers
  • Limit min/max zoom scale extent
  • Limit min/max zoom translate extent
  • Use manual ranking in Dagre
  • Get manual ranking merged into Dagre
  • Calculate manual ranks on the client side
  • Demonstrate switching between rankers in Dagre
  • Remove temporary ranker switch toggle
  • Fix layer label overlaps
  • Fix transition delay bug
  • Fix tests
  • Add layer toggle button
  • Update documentation

QA notes

There's a new test dataset called 'layers.mock.js'. To see layers in action, add ?data=layers to the URL.

You can toggle them on/off with this button:
image

You can see the layer bands by hovering over them. Note that they're not shown when hovering over a node - @GabrielComymQB is this okay?

Checklist

  • Read the contributing guidelines
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added new entries to the RELEASE.md file
  • Added tests to cover my changes

Legal notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

  • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.

  • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.

  • I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

There are a few features that I've added for testing, which will be removed before merging
@richardwestenra richardwestenra marked this pull request as ready for review April 3, 2020 10:35
src/selectors/disabled.test.js Show resolved Hide resolved
src/selectors/layers.js Show resolved Hide resolved
src/selectors/layers.js Show resolved Hide resolved
src/selectors/layers.test.js Show resolved Hide resolved
src/selectors/layout.js Outdated Show resolved Hide resolved
expect(getZoomPosition(mockState.lorem)).toEqual(defaultZoom);
});

it('returns default values if graph width/height is infinite (i.e. when no nodes are visible)', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's a straightforward change, I think using 0 width for an empty graph might be clearer than Infinity?

Copy link
Contributor Author

@richardwestenra richardwestenra Apr 6, 2020

Choose a reason for hiding this comment

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

Infinity is coming from Dagre: It's returned when the graph has 0 nodes/edges. This happens when you disable all the nodes.

I could just avoid running Dagre layout at all and return undefined when nodes.length === 0? I'd just have to add some extra checks on the functions that receive the output. What do you reckon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: have done it. See 78dbf90

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah well if you agree that it seems more natural, I think it would be nice to catch that Infinity early on if it's a small change, either like you say or swap it with 0 at source?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Done!

@richardwestenra richardwestenra merged commit 4e597dd into develop Apr 7, 2020
@richardwestenra richardwestenra deleted the prototype/custom-ranking branch April 7, 2020 13:49
@richardwestenra richardwestenra mentioned this pull request Apr 9, 2020
6 tasks
@richardwestenra richardwestenra mentioned this pull request May 26, 2020
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