search

mstefaniuk / graph-viz-d3-js

Graphviz web D3.js renderer
MIT License
313 stars 38 forks source link

Upgrade of, and adaption to D3 to version 4 #39

Closed magjac closed 7 years ago

magjac commented 7 years ago

You may want to have a look at the latest commit, 495567b4d3c3a1afe8ec80374831ecea15bb644b. I really don't understand why it is needed for labels (text), but not for shapes (path). The tests ran fine without it, but the initial transition was very funny, with shapes dropping down linearly from the top, but labels spreading of from the top left corner.

I've done a few manual checks with my local graphviz.it, but I would appreciate if you could also do some.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.4%) to 96.682% when pulling 495567b4d3c3a1afe8ec80374831ecea15bb644b on magjac:master into 3c0a0699bdaa1ff653a513d5a1cece57c4f4bff0 on mstefaniuk:master.

magjac commented 7 years ago

I'll have a look at the decreased coverage later.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.5%) to 97.63% when pulling bfb1e5ff8908d16f6d508b87b5e636478ee580cd on magjac:master into 3c0a0699bdaa1ff653a513d5a1cece57c4f4bff0 on mstefaniuk:master.

mstefaniuk commented 7 years ago

@magjac Forget so small coverage drop. It may be caused by need to add selection argument to selection manipulation functions. Please look into my review comment. After fixing that I will merge PR with pleasure.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-0.5%) to 97.63% when pulling cbd52fe26dde03c804dcc55506fbf9cd9cea57c0 on magjac:master into 3c0a0699bdaa1ff653a513d5a1cece57c4f4bff0 on mstefaniuk:master.

magjac commented 7 years ago

Now a better solution in stage without dependency to styliseur. Please see also my comments to your review regarding possible bug.

mstefaniuk commented 7 years ago

Maybe I expected you to fix bug because you introduced new logic in label rendering and most of the changes has been related to new d3 API but not this one.

magjac commented 7 years ago

If there is a bug, it was introduced or revealed by the change from D3 v3 to v4. There was no problem before that. You can see for yourself as I explained in my comment to your initial review comments.

If there is a bug, my change is not a fix, but a workaround. But since I don't understand how initial transitions (i.e. for enter selections) work even for the shapes which do not have any workaround, I don't know if it is the correct way or not. That's why I wanted you to review it.

Anyway, thanks for accepting my PR 😃.