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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Customize color for a link #79

Merged

Conversation

@LonelyPrincess
Copy link
Collaborator

LonelyPrincess commented Jun 23, 2018

I was planning on using your library for a project I'm currently working on, but I needed to be able to customize the color for each link separately. I saw a recently closed issue requesting this feature (#71) and that there was no one who had offered to implement it yet, so I decided to give it a go.

This set of changes is the result, and it makes it possible to set a custom color for single link when defining its data:

{ source: 'A', target: 'B', color: 'orange' }

A couple of tests were also included to ensure the behaviour is as expected:

  • If a custom color was set for the link, the stroke value for the line to draw must be equal to said color.
  • If no custom color was provided, the stroke value should match the one defined in the link configuration object.

PS: Very beautifully written and documented code, it was a real joy to work with. Congratulations and thanks for your hard work! 馃槈

When adding links to a dataset now, we can add optionally include a 'color'
property. If it's set, said color will override the default color
defined in the configuration object.

In order for this to work, the 'buildLinkProps' is now receiving a 'link' object, which could contain any additional data aside from the source and target nodes. In this case, we'll be only considering a 'color' property that will be taken into account when setting the stroke color of the line to draw.

The former 'source' and 'target' parameters that used to be stand-alone parameters in the function, are now data contained inside of the
new 'link' object.
Added some new tests in the 'graph.helper.test' suite to check whether
the customization of the color for a certain link is working correctly.

We will be checking whether the stroke color matches the expected one in
both scenarios: when a custom color is set and when no color has been
passed along the link data.
@LonelyPrincess

This comment has been minimized.

Copy link
Collaborator Author

LonelyPrincess commented Jun 25, 2018

Hi there! Thanks for the feedback, @danielcaldas!

It doesn't seem to work just by doing what you say, though. It didn't for me when I run the sandbox example, at least. 馃槥

Both target and source properties in link are objects, which causes buildLinkProps to fail. That method is expecting node ids instead, which is why you were defining a sourceId and targetId variables before and passing them down to buildLinkProps. Here you have an example on what link looks like at that point in the code:

{
   "source": {
       "id":"VUX",
       "x":367.86084524307813,
       "y":261.26222418545444,
       "highlighted":false,
       "index":6,
       "vy":-0.0009596293512956998,
       "vx":0.0011615622407413048
   },
   "target":{
       "id":"Intruder",
       "symbolType":"square",
       "x":262.27673946872176,
       "y":334.2477074137166,
       "highlighted":false,
       "index":13,
       "vy":-0.001946020267134013,
       "vx":-0.000017777191095397,
       "fx":262.27673946872176,
       "fy":334.2477074137166
   },
   "color":"orange",
   "index":22
}

Is there any case in which you'll actually be receiving a string with the id instead of an object? You had this on the code implying it is a possibility, but it doesn't seem like it can actually happen now.

const sourceId = source.id || source;

Also, you had this comment on line 30:

// FIXME: solve this source data inconsistency later

Could this have something to do with the change you are proposing? 馃

At any rate, if we want to pass link as it is to buildLinkProps, doing it shouldn't be a problem as long as we prepare that method to work with it. Two alternatives come to mind now:

  1. Obtain ids for both source and target nodes at the beginning of the function. These ids will be used as indexes to retrieve data from nodes.
const { source, target } = link;
const sourceId = source.id || source;
const targetId = target.id || target;

const x1 = (nodes[sourceId] && nodes[sourceId].x) || 0; 

[...]

return {
        sourceId,
        targetId,
        [...]
}
  1. Since link is already including data on the nodes it's connecting, we could avoid passing nodes to the function and get the information we need straight from the link object.
const { source, target } = link;

const x1 = (source && source.x) || 0;

[...]

return {
        source.id,
        target.id,
        [...]
}

This one would work as long as target and source are always objects. If there is any scenario in which link.source is just an string with an id instead, then the first approach should be a safer bet.

Does any of these choices look right to you?

@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Jun 25, 2018

Hey thanks for providing the options but I prefer the one you have currently where source and target override link ones making it string identifiers. I actually deleted my comment right away when I remembered that source and target are objects, but somehow you were able to read the comment. I'm happy with your current approach, let's move forward with it 馃憤

@danielcaldas danielcaldas merged commit a515806 into danielcaldas:master Jun 25, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
danielcaldas added a commit that referenced this pull request Jun 25, 2018
* Customizable color for individual links

When adding links to a dataset now, we can add optionally include a 'color'
property. If it's set, said color will override the default color
defined in the configuration object.

In order for this to work, the 'buildLinkProps' is now receiving a 'link' object, which could contain any additional data aside from the source and target nodes. In this case, we'll be only considering a 'color' property that will be taken into account when setting the stroke color of the line to draw.

The former 'source' and 'target' parameters that used to be stand-alone parameters in the function, are now data contained inside of the
new 'link' object.

* Add tests for custom link feature

Added some new tests in the 'graph.helper.test' suite to check whether
the customization of the color for a certain link is working correctly.

We will be checking whether the stroke color matches the expected one in
both scenarios: when a custom color is set and when no color has been
passed along the link data.
@LonelyPrincess LonelyPrincess deleted the LonelyPrincess:feat_custom_link_color branch Sep 20, 2018
@wendymungovan

This comment has been minimized.

Copy link

wendymungovan commented Jan 30, 2019

This is a great feature. Thank you for adding it. It works great the first time we setup the graph but when we add new nodes and link the link go back to being the default color and not using the setting set in the edge's configuration. Is there something extra I need to do to have it use the link's color?
Thank you.

@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Jan 30, 2019

Hi, @wendymungovan thanks for the feedback please submit an issue with your request and try to explain the problem with a little more detail.

@danielcaldas

This comment has been minimized.

Copy link
Owner

danielcaldas commented Oct 2, 2019

FYI @wendymungovan @LonelyPrincess the issue that @wendymungovan is referring to in the comment above (Jan 30), was fixed in this PR #220. This was released with react-d3-graph v2.2.0

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鈥檛 perform that action at this time.