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

Specify links in Graph component update #186

Merged
merged 3 commits into from Mar 28, 2019

Conversation

@grant37
Copy link
Contributor

grant37 commented Mar 23, 2019

I think this might fix #183. I was having the same problem when I was trying to update a graph by modifying the data prop in order to reduce the graph to a subset of the original. The right nodes would render but the links were sort of hit or miss. For me the issue was that d3Links[index] in _mapDataLinkToD3Link isn't necessarily going to be the same as nextProps.data.links[index]. Using the find function instead should make sure that they match. It's worked so far for my use cases. The change doesn't seem to break any tests, and I added one test that fails without the change but passes with it. I'm happy to add tests or make follow up changes if anyone is interested in merging this.

Copy link
Owner

danielcaldas left a comment

This is looking good, it seems that this missing index was caused by a refactored that moved some logic around in this method. Nice catch @grant37

const customProps = utils.pick(link, LINK_CUSTOM_PROPS_WHITELIST);

if (d3Link) {
const toggledDirected = state.config && state.config.directed && config.directed !== state.config.directed;
const refinedD3Link = {
...d3Link,
index,

This comment has been minimized.

Copy link
@danielcaldas

danielcaldas Mar 27, 2019

Owner

This is correct the index should be passed into the link object as it's done in line 171 when we return.

This comment has been minimized.

Copy link
@grant37

grant37 Mar 27, 2019

Author Contributor

Ah cool, thanks for taking a look! Sorry for splitting this into two commits - at first I was letting the update use the old index.

@danielcaldas danielcaldas merged commit 6168c28 into danielcaldas:master Mar 28, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sibbl

This comment has been minimized.

Copy link

sibbl commented Apr 23, 2019

Sorry that I open this again, but is there any ETA when this fix will be released (in terms of a new npm version?)

@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Apr 25, 2019

Hello, @sibbl I will release a patch version soon (should be v2.0.2) :)

@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Apr 27, 2019

Hey there @sibbl @grant37 just released react-d3-graph@2.0.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.