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 more events to be handle on d3-network components #12

Closed
wants to merge 18 commits into from

Conversation

vibou
Copy link

@vibou vibou commented Nov 23, 2017

New events handled:

sim-start
sim-tick
sim-end

mouse-enter-node
mouse-leave-node
mouse-enter-link
mouse-leave-link

@emiliorizzo
Copy link
Owner

emiliorizzo commented Nov 23, 2017

First, thanks for contribute to this project.
Your pull request introduces two modifications:

  • Adds ids to svg nodes
  • Add events to svg nodes and simulation

The addition of ids is unnecesary, you can add ids on _svgAttrs object.
For example:

data () {
    return {
      nodes: [
        { id: 1, name: 'my node 1',_svgAttrs:{ id:'node-1' } },

.....
Or using custom node formatter:

  formatNode(node){
    let svgAttrs = node._svgAttrs || {}
    if(!svgAttrs.id) svgAttrs.id = 'node-'+ node.id
    node._svgAttrs = svgAttrs
    return node
  }

https://jsfiddle.net/emii/wq6stonL/

About events: I didn't have time to analyze this.
But I am not a supporter of add too much events listeners
I will try to find another solution and answer soon.

@vibou
Copy link
Author

vibou commented Nov 29, 2017

Hi. I agree with you for the id. So I will remove it however I will let the newly handle events. In addition I am looking for a way to add text on links. I will let you know if I succeed :)

@vibou
Copy link
Author

vibou commented Nov 29, 2017

Text on links image
It is not the best implementation to add text on links. It would be better to transform orientation. Still working on it.

@@ -84,9 +83,18 @@
:x='node.x + (getNodeSize(node) / 2) + (fontSize / 2)'
:y='node.y + labelOffset.y'
:font-size="fontSize"
:class='(node._labelClass) ? node._labelClass : ""'
:class='(node._labelClass) ? node._labelClass : "node-label"'
Copy link
Owner

Choose a reason for hiding this comment

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

why?.... text.node-label.....

emiliorizzo added a commit that referenced this pull request Nov 29, 2017
@emiliorizzo
Copy link
Owner

@vibou
Copy link
Author

vibou commented Nov 30, 2017

I don't understand why you removed the sim events. I understand it is now possible for the user to use simCb to add his own event using the callback but it is somehow counter intuitive. Your component should be easy to use and give the developpers features without having him looking at every doc of the internal components you use (in my view).

If tomorrow another lib is used internally to generate the graph, then the developper will have to rewrite his code to support sim-start / sim-end and sim-tick events.

Eventually I manage to have labels on the links as you did. I did not look at how you implemented it. I guessed you used the same method as I (using path instead of line and textPath). However I ve seen another use case that allows developper to have up to 3 labels onto a link.

I think name (for node as well as for link) should be rename into label.

In my solution, link.name can be either a string or an array of length 1, 2 or 3

switch(length)
   case 1:
         label[0] is aligned in the middle
   case 2:
         label[0] is aligned next to the source
         label[1] is aligned next to the target
  case 3:
        label[0] is aligned next to the source
        label[1] is aligned in the middle
        label[2] is aligned next to the target

Let me know what do you think about all of this. Plus it could be great to close the request if you implement everything that is in this request.

@vibou
Copy link
Author

vibou commented Nov 30, 2017

I perform a merge and re-compile the files this should be ok. Here is the list of new features:

  • Handle mouse events (enter/leave node/link)
  • Handle simulation events
  • Labeling links (with up to 3 labels)

@vibou
Copy link
Author

vibou commented Nov 30, 2017

I have one issue with the canvasRendered where labels are not rendering. I did not investigate on this point since I never worked on the canvasRenderer.

@emiliorizzo
Copy link
Owner

emiliorizzo commented Nov 30, 2017

I don't understand why you removed the sim events. I understand it is now possible for the user to use simCb to add his own event using the callback but it is somehow counter intuitive. Your component should be easy to use and give the developpers features without having him looking at every doc of the internal components you use (in my view).

If tomorrow another lib is used internally to generate the graph, then the developper will have to rewrite his code to support sim-start / sim-end and sim-tick events.

I think that the simCb option open more possibilities for set the simulation.
But I agree with you, about this point.

@emiliorizzo
Copy link
Owner

emiliorizzo commented Nov 30, 2017

I have one issue with the canvasRendered where labels are not rendering. I did not investigate on this point since I never worked on the canvasRenderer.

The link labels does not work on canvas yet.
In canvas is not possible to align text to path, I need find a solution to this.

@emiliorizzo
Copy link
Owner

emiliorizzo commented Nov 30, 2017

@vibou In the future, please, separate your pull request by features.
About the mouse events, I dont want add unnecessary listeners, the mouse events must be optionals. What about canvas? Do you thinks how to handle nodes and links events?

@vibou
Copy link
Author

vibou commented Dec 1, 2017

Great. Yeah next feature I ll do another branch. Thanks for your lib which is great ! :P

@vibou vibou closed this Jan 9, 2018
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.

None yet

2 participants