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

Avoid creating orphan edges #99

Merged
merged 2 commits into from
Feb 27, 2019

Conversation

iamsoorena
Copy link
Contributor

before removing these lines, If user didn't make a new edge with onCreateEdge, we would have orphan edges.

Demo of bug:
orphaned-edge

@ajbogh
Copy link
Contributor

ajbogh commented Dec 13, 2018

This is an interesting use case. Thanks for showing it in an image!

I worry if the right approach is to remove those lines. I'll have to see what the implications are.

Another way to handle this is to call this.setState() again to force react-digraph to rerender with the existing graph, which should delete the extra edge. You would do this when the user presses the cancel button.

@iamsoorena
Copy link
Contributor Author

iamsoorena commented Dec 15, 2018

Hi there, thanks for your response.
the graph works just fine without those lines.
I think the only reason they exist is for graph to update faster, because it sometimes takes a while for graph to re-render new edges.
If that's the case we can have some extra methods for adding edges asynchronously to avoid these kinds of conflicts.

@iamsoorena
Copy link
Contributor Author

@ajbogh This is critical, the graph won't re-render only by calling this.setState().
this orphaned edge won't go away even if I set completely new edges => this.setState({edges: currentEdges.slice()})!!! that's unacceptable.
So this orphaned edge does not go away.

@ajbogh
Copy link
Contributor

ajbogh commented Jan 11, 2019

Hi @iamsoorena. I plan to look into this as soon as I am complete with another priority. I apologize for the delay and the rest of the team is split with their priorities as well. Please allow me to look into this next week.

@iamsoorena
Copy link
Contributor Author

Hi @ajbogh, we are using this library in a serious project. I have tested library without those lines a few times. there is no problem with removing those lines. those lines only introduce some buggy behavior and I will be happy If you find the time to look into this.

@iamsoorena
Copy link
Contributor Author

@ajbogh Hi.

@ajbogh
Copy link
Contributor

ajbogh commented Jan 31, 2019

@iamsoorena I've asked another team member to look into this. Hopefully he can get to it soon. Thanks for the patience.

@vincentuber

@ajbogh ajbogh added the bug label Jan 31, 2019
@vjocw
Copy link
Contributor

vjocw commented Feb 1, 2019

@ajbogh @iamsoorena looking into it now. I'm getting now ramped up with the codebase, it might take some time.

@ahslr
Copy link

ahslr commented Feb 8, 2019

Hey @chiwoojo
Did you have a chance to look at this?

@ajbogh
Copy link
Contributor

ajbogh commented Feb 27, 2019

I was able to test this today and found that it works fine for existing projects. Thanks for being so patient with this, and I apologize for the delay.

@ajbogh ajbogh merged commit 1ce3f3a into uber:master Feb 27, 2019
@ajbogh
Copy link
Contributor

ajbogh commented Feb 27, 2019

This is included in 6.2.0. Thanks!

@Malikx-Alee Malikx-Alee mentioned this pull request Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants