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

Fixed #219 of upstream. Can now render multiple graphs at the same time. #234

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

ajbogh
Copy link
Contributor

@ajbogh ajbogh commented Jul 17, 2020

  • Fixed rendering and element selection to allow for multiple graphs on the same webpage.
  • Fixed various edge rendering issues as well, particularly noticeable in the Transformer example.
  • Cleaned up the examples.

This PR should require a major version change due to breaking changes in the CSS for the zoom controls.

Screenshot_2020-07-17_00-21-00

// Although it doesn't allow multiple graphs.
// We can use viewWrapperElem to scope the querySelector to a smaller set of elements to improve the speed
const nodeElem = viewWrapperElem.querySelector(
`[id='node-${trg[nodeKey]}']`
Copy link
Contributor

Choose a reason for hiding this comment

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

if we’re doing breaking changes, should we change ids to classes so we don’t need to use these workarounds for duplicate ids within the same document?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps in the next PR we can address that. Please feel free to create a new PR after this if you want.

You are right that this will be a breaking change, so we have the opportunity to make a number of improvements.

const graphControlsWrapper = this.viewWrapper.current.ownerDocument.getElementById(
'react-digraph-graph-controls-wrapper'
const graphControlsWrapper = this.viewWrapper.current.querySelector(
'#react-digraph-graph-controls-wrapper'
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

@ajbogh ajbogh merged commit a8cc2d6 into uber:master Jul 27, 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.

3 participants