PoC - d3v4 support #1363

kum-deepak · 2018-02-18T17:45:56Z

Reference:
#1173

gordonwoodhull · 2018-02-18T18:00:41Z

Thanks @kum-deepak! This sounds like a good approach.

The selection behavior has also changed a lot, since d3v3 used to automatically merge the enter selection into the modify selection. I guess maybe your compatibility layer can handle this, but when eventually removing the compatibility layer, it is still going to be tricky.

Please continue to update with your progress and I'll be happy to review or help out as you go.

kum-deepak · 2018-02-20T04:36:23Z

Thanks for your inputs. I have been meditating for past two days on best way to handle changes to selection behavior.

.enter() is currently used in 36 places. Writing a wrapper is possible but quite clumsy, the effort to test the wrapper is going to be more than direct changes (which anyway will need to be done).

I understand that it is not pragmatic to maintain support for both versions in the same branch. At the same time due to nature of changes it is likely that support for D3v3 and D3v4 will need to co-exist for significant time.

So, I was thinking - as far as possible - to modify in the code in a way that modified code works with D3v3 as well. So far there are following known changes

accessing nodes from a selection
behavior of selectAll in regard to .enter()

I think both of these can be created a functions that can be implemented for D3v3 and D3v4.

I will commit again when I reach someplace.

gordonwoodhull · 2018-02-20T05:16:47Z

I think it's perfectly acceptable to simply move to d3v4. This was how I planned to make the port when I find time.

It's true, there will be some overlap, and fixes will have to be applied separately to two branches for a while. But once we get d3v4 fully working, it should be fine to drop d3v3, since the rest of the world has already moved on.

My two cents. Thank you for taking this challenge, and feel free to pursue it however you think is best.

kum-deepak · 2018-02-20T05:39:07Z

Thanks! I will proceed with making direct changes. That will actually make the changes simpler :)

I was going through the code - the more complex changes brush() and dispatch() are used only once each.

kum-deepak · 2018-02-20T05:39:42Z

Your message was really timely :)

gordonwoodhull · 2018-02-20T06:27:29Z

Great! I think the scatter plot modifies the brush a little bit too, but otherwise yes that should be it.

Fixed test cases

kum-deepak · 2018-02-25T08:52:50Z

Have been able to get more chart types to work. One setback - the stack API has changed beyond recognition. Spent some time on that but was not able to move it. Good thing, it is a relatively isolated code in D3v3, extracted relevant part and placed in the d3-compat file. That has allowed me to go ahead. I will come back to it when other issues are under control.

Next target for me is brushes.

gordonwoodhull · 2018-02-25T16:49:45Z

Awesome! Yes, having read CHANGES.md, it looks like every interface has changed somewhat, and many in fundamental ways.

So grateful you are taking the time to work on this.

kum-deepak · 2018-02-27T02:33:32Z

Brushing itself works :) New brush APIs are simpler and is likely to yield a simpler code.

I am facing a very stupid doubt, brush needs to be told the region brushing is allowed. For this I need the width and height of the chart area. How do I get that?

gordonwoodhull · 2018-02-27T12:58:51Z

That's great!

You can use .effectiveWidth() and .effectiveHeight() to get the chart extent. They are defined in the marginMixin.

kum-deepak · 2018-02-28T09:41:12Z

Started seeing end of the tunnel. Many examples have started working. Key pending dev work:

Zoom
Brush - need to implement 2D brushing for scatter plots, refactor general brush
Area - lines are working, area is not

Examples that are not working as of now:

http://localhost:8888/web/examples/area.html (lines ok, area missing)
http://localhost:8888/web/examples/box-plot-time.html
http://localhost:8888/web/examples/line.html (lines ok, area missing)
http://localhost:8888/web/examples/multi-focus.html
http://localhost:8888/web/examples/range-series.html (brush/zoom not working)
http://localhost:8888/web/examples/scatter-brushing.html (brush not working)
http://localhost:8888/web/examples/series.html
http://localhost:8888/web/examples/splom.html (brush not working)
http://localhost:8888/web/examples/time-intervals.html
(bar width, some warnings, brushing not working)

gordonwoodhull · 2018-02-28T14:08:42Z

Wow! You're really getting there!

Amazing work.

kum-deepak · 2018-02-28T16:35:33Z

Zoom and focus works now. :)

gordonwoodhull · 2018-03-17T14:48:23Z

Completely agree. Looks like we're almost there - congratulations!

gordonwoodhull · 2018-03-17T21:59:25Z

I was able to do some testing today, so I ran through all the examples.

Here are the issues I found, mostly to do with initial values for elements that are entering.

We don't need to fix these before putting out the alpha, but we'll definitely need to fix them, as well as using the d3v4 stack implementation, before releasing 3.0.0. I'll open issues for any that you don't fix before we merge this PR.

bar charts

introductory transition on bars is from lower left instead of just from bottom
bar labels transition from upper left instead of just being there at start
multi focus example: bars turn grey before being filtered/focused (?) out instead of just disappearing. Not sure which behavior is better here, but we should figure out what is going on here and decide one way or the other.

line charts

dots transition from upper left instead of just being there
range series example: brush handles not removed when clicking to remove brush. probably because composite chart?

heat map

squares transition from upper right instead of just fading in

Transitions have many, many issues, but at least we shouldn't regress with this release.

kum-deepak · 2018-03-18T01:43:43Z

Thanks for your review.

I have committed now draft version of upgrade guide. I now realize that other than changes to the user code for d3 changes, there are only two changes.

It is possible to make those two backward compatible, will wait for your feedback.

kum-deepak · 2018-03-18T01:53:22Z

I think we have reached a stage where we can baseline it as a single commit, and split for work on localized/specific issues.

My current (non exhaustive list):

Cleanup of zoom behavior. Testing of restrict panning etc.
dc.event.trigger
exit/enter/update sequence - this is an important one
Additional test cases for dc.util.add and dc.util.subtract
One for each new chart type - original PRs from community
Consider removing transitions for zooming and brushing
d3 stack to D3v4
Changing .tension call in lineChart
http://localhost:8888/web/examples/filter-stacks.html
(fading is peculiar)
http://localhost:8888/web/zoom/restrict-panning.html
Need to increase height of range chart - currently height of
brushable area is zero. New implementation needs it be more than zero.
Issues with transitions (d3 behavior might have changed, or I clobbered some functionality as I did not understand that)

I will describe each of these on the individual issue/PR.

I will keep working one/few at a time. Will love to hear your prioritization/grouping of the above.

kum-deepak · 2018-03-18T02:12:26Z

It was my first experience with working with D3 - completing a month now. :)

I now realize that behavior of .selectAll in D3v4 implicitly favors code that first handles exiting elements, following by entering and updating. The current dc code does it in sequence enter, update and exit.

Making the change will make the code cleaner and more manageable. However I am not sure if will make any adverse noticeable change for the user. In any case I wanted first a working version before attempting this one.

In the current code sometimes, .enter is called in a function other than the function that called .selectAll. I was needed to return the entering collection from the function as it it is needed for the call to .merge. A more maintainable code will be to call .enter in the same function as .selectAll and adjust the parameters accordingly.

I think that when we make the above change - some subtle bugs would get fixed - which may be related to updating elements not behaving properly.

Will wait for your suggestion before proceeding on this one.

kum-deepak · 2018-03-20T16:19:49Z

@gordonwoodhull can you please merge this pull request as a new branch (say d3v4) as a single commit. I will use that as base to continue further work.

Thanks!

gordonwoodhull · 2018-03-20T16:28:46Z

Yes I'll create a 3.0 branch today. Sorry for the delay - yesterday I got bogged down thinking about whether we should be backward compatible. (It's easy enough so I think we should.)

Please start opening issues for the remaining work. I'll create 3.0 milestone now.

kum-deepak · 2018-03-20T16:35:14Z

After having done everything, looks like we can actually be backward compatible, only two changes, which can both be extended to mimic the current API behavior.

As we can move towards 3.0 we can start putting deprecation notices as suggestions to update.

kum-deepak · 2018-03-20T16:37:51Z

There may still remain some incompatibility around tension in interpolate/curve function. Which may impact very few users.

Squashed from #1363 pie-charts all test cases passing with d3v4 Better alignment to build infrastructure .selectAll('anything')[0] -> .selectAll('anything').nodes() interpolate and tension semantics have changed. Updated for semantics of D3v4 .enter() Fixed test cases Discovered that more D3 functions that are used. Updated for semantics of D3v4 .enter() Updated for semantics of D3v4 .enter() and fixed test cases. All test cases for number display and row chart passing Tests passing Axis, Scales - temp Added d3-compat to examples Brushing - initial work Many examples working :) Zoom and focus working Home page charts works (issue with brushes) Fixing some of the specs X Axis brushing Brushing for Scatter Plot Brush extents and background color Brush extent specs Brushing works :) Current status Bar width corrected Area is working geoChoroplethChart Functional completeness dispatch directly implemented, compatibility code discarded. base-mixin test cases passing All specs passing All specs passing Merged D3v3-compat Minor cleanup Merge d3 compat Cleanup brush related code Updated test case Travis Inline docs - d3 documentation links and minor changes Language correction Upgrade guide draft Update copyright notice Cleanup

gordonwoodhull · 2018-03-21T17:48:37Z

Sorry for the delay - I'm still not able to work much. I've created the 3.0 branch now!

BTW you're not following the jshint/jscs style guides. I guess maybe you never ran plain grunt with no parameters, which installs some useful commit hooks. If you don't want the hooks just run grunt lint to check everything. It doesn't matter to me if those don't pass every time, but you might want to start out with a single commit to fix these since it will touch a lot of files.

From now on, please select 3.0 as the base for your future PRs. I'll close this PR now, although there has not been an official release yet.

gordonwoodhull · 2018-04-02T18:24:28Z

src/bubble-chart.js

@@ -123,9 +128,9 @@ dc.bubbleChart = function (parent, chartGroup) {
        // override default x axis brush from parent chart
    };

-    _chart.redrawBrush = function () {
+    _chart.redrawBrush = function (g, selection, doTransition) {


Whoops - looks like you added some arguments here but never pass them anywhere. I just ran into this because I wanted to add a dontSetBrush argument. Do you remember what you were trying to do here?

Specifically, it looks like you changed the signature to take the current filter/selection, but this also takes g and doTransition - so I guess it's a simple fix since those are unused.

gordonwoodhull · 2018-04-02T19:50:58Z

src/coordinate-grid-mixin.js

-            if (_chart.filter() && _chart.brush().empty()) {
-                _chart.brush().extent(_chart.filter());
-            }
+    _chart.redrawBrush = function (selection) {


Why did you remove doTransition here? _brush.move can take a transition or a selection, just like the old brush could be applied to either a selection or transition.

It's not a big deal, just a tiny feature that makes chart resizing work better, but it makes me worry when functionality is removed with no explanation. In the future, could you please try to document the things that you disable or remove?

I guess I need to go over this PR with a fine-toothed comb before we release 3.0. We don't have full test coverage, especially where transitions are concerned - it's unclear how to test them.

Thanks for letting me know, I will mark in future if I remove or change any functionality.

This one, in my initial implementation, was aggravating the infinite recursion. However the current code, may not suffer from that.

I will be more comfortable if you could go through the entire change set considering it to be my first encounter with both dc and d3.

I see, you probably simplified it in order to figure out why it was recursive, and then didn't restore the feature when you were done.

No harm done. Yes, I'll read over all the changes before we release.

Justus-Maier · 2018-04-03T08:14:10Z

@gordonwoodhull how far are you through code review? Would you publish an alpha asap please?

kum-deepak · 2018-04-03T08:18:45Z

@Justus-Maier there are lot of changes, so it will take a while. Meanwhile you can install from 3.0 branch. Code can be considered alpha.

gordonwoodhull · 2018-04-03T12:39:52Z

Yes, as @kum-deepak says, review is taking a while. Although I've started giving them alpha numbers (we're at 2 going on 3) I am not sure when I will publish to npm & cdnjs. Hopefully later this week.

(The trouble is that a lot of people will take the latest version even if it says alpha, and then complain about it. Bug reports are welcome of course!)

Meanwhile, if you're using npm, you can change your dependency in package.json to:

    "dc": "git://github.com/dc-js/dc.js#3.0",

to install directly from the github branch.

gordonwoodhull · 2018-04-07T14:47:13Z

src/composite-chart.js

+     * unexpected results.  Note also that when used as a getter, this function is not chainable: it
+     * returns the axis, not the chart,
+     * {@link https://github.com/dc-js/dc.js/wiki/FAQ#why-does-everything-break-after-a-call-to-xaxis-or-yaxis
+     * so attempting to call chart functions after calling `.yAxis()` will fail}.


pie-charts all test cases passing with d3v4

bb7b419

kum-deepak mentioned this pull request Feb 18, 2018

Support D3 v4 #1173

Closed

Better alignment to build infrastructure

09698c5

kum-deepak added 9 commits February 20, 2018 15:35

.selectAll('anything')[0] -> .selectAll('anything').nodes()

771a714

interpolate and tension semantics have changed.

3cc0ee2

Updated for semantics of D3v4 .enter()

9253b90

Fixed test cases

Discovered that more D3 functions that are used.

159e768

Updated for semantics of D3v4 .enter()

e86896f

Updated for semantics of D3v4 .enter() and fixed test cases.

d3feee0

All test cases for number display and row chart passing

2d624bb

Tests passing

5561b74

Axis, Scales - temp

7d8e196

kum-deepak added 2 commits February 25, 2018 21:38

Added d3-compat to examples

0d3c238

Brushing - initial work

fc59bdd

Many examples working :)

e18a8bb

Zoom and focus working

2d082ee

Home page charts works (issue with brushes)

d5b4b26

kum-deepak added 2 commits March 17, 2018 20:50

Language correction

f7d6c21

Upgrade guide draft

c29d5ff

Update copyright notice

b597177

Cleanup

84f1608

gordonwoodhull closed this Mar 21, 2018

gordonwoodhull mentioned this pull request Mar 24, 2018

reordering exit/enter/update #1380

Open

kum-deepak deleted the d3v4 branch March 28, 2018 12:37

gordonwoodhull mentioned this pull request Mar 28, 2018

Adding support for dc.lineChart.curve. Deprecating interpolate/tension. #1381

Closed

gordonwoodhull reviewed Apr 2, 2018

View reviewed changes

gordonwoodhull added a commit that referenced this pull request Apr 2, 2018

fix redrawBrush signature for #1363

59e0473

gordonwoodhull reviewed Apr 7, 2018

View reviewed changes

This was referenced Apr 7, 2018

d3v4 cleanup #1398

Closed

fix entering transition positions #1400

Closed

exiting bars turn grey first #1401

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PoC - d3v4 support #1363

PoC - d3v4 support #1363

kum-deepak commented Feb 18, 2018

gordonwoodhull commented Feb 18, 2018

kum-deepak commented Feb 20, 2018

gordonwoodhull commented Feb 20, 2018

kum-deepak commented Feb 20, 2018

kum-deepak commented Feb 20, 2018

gordonwoodhull commented Feb 20, 2018

kum-deepak commented Feb 25, 2018

gordonwoodhull commented Feb 25, 2018

kum-deepak commented Feb 27, 2018

gordonwoodhull commented Feb 27, 2018

kum-deepak commented Feb 28, 2018 •

edited

Loading

gordonwoodhull commented Feb 28, 2018

kum-deepak commented Feb 28, 2018

gordonwoodhull commented Mar 17, 2018

gordonwoodhull commented Mar 17, 2018 •

edited

Loading

kum-deepak commented Mar 18, 2018

kum-deepak commented Mar 18, 2018

kum-deepak commented Mar 18, 2018

kum-deepak commented Mar 20, 2018

gordonwoodhull commented Mar 20, 2018

kum-deepak commented Mar 20, 2018

kum-deepak commented Mar 20, 2018

gordonwoodhull commented Mar 21, 2018

gordonwoodhull Apr 2, 2018 •

edited

Loading

gordonwoodhull Apr 2, 2018

gordonwoodhull Apr 2, 2018

kum-deepak Apr 3, 2018

gordonwoodhull Apr 3, 2018

Justus-Maier commented Apr 3, 2018 •

edited

Loading

kum-deepak commented Apr 3, 2018

gordonwoodhull commented Apr 3, 2018 •

edited

Loading

gordonwoodhull Apr 7, 2018

PoC - d3v4 support #1363

PoC - d3v4 support #1363

Conversation

kum-deepak commented Feb 18, 2018

gordonwoodhull commented Feb 18, 2018

kum-deepak commented Feb 20, 2018

gordonwoodhull commented Feb 20, 2018

kum-deepak commented Feb 20, 2018

kum-deepak commented Feb 20, 2018

gordonwoodhull commented Feb 20, 2018

kum-deepak commented Feb 25, 2018

gordonwoodhull commented Feb 25, 2018

kum-deepak commented Feb 27, 2018

gordonwoodhull commented Feb 27, 2018

kum-deepak commented Feb 28, 2018 • edited Loading

gordonwoodhull commented Feb 28, 2018

kum-deepak commented Feb 28, 2018

gordonwoodhull commented Mar 17, 2018

gordonwoodhull commented Mar 17, 2018 • edited Loading

bar charts

line charts

heat map

kum-deepak commented Mar 18, 2018

kum-deepak commented Mar 18, 2018

kum-deepak commented Mar 18, 2018

kum-deepak commented Mar 20, 2018

gordonwoodhull commented Mar 20, 2018

kum-deepak commented Mar 20, 2018

kum-deepak commented Mar 20, 2018

gordonwoodhull commented Mar 21, 2018

gordonwoodhull Apr 2, 2018 • edited Loading

Choose a reason for hiding this comment

gordonwoodhull Apr 2, 2018

Choose a reason for hiding this comment

gordonwoodhull Apr 2, 2018

Choose a reason for hiding this comment

kum-deepak Apr 3, 2018

Choose a reason for hiding this comment

gordonwoodhull Apr 3, 2018

Choose a reason for hiding this comment

Justus-Maier commented Apr 3, 2018 • edited Loading

kum-deepak commented Apr 3, 2018

gordonwoodhull commented Apr 3, 2018 • edited Loading

gordonwoodhull Apr 7, 2018

Choose a reason for hiding this comment

kum-deepak commented Feb 28, 2018 •

edited

Loading

gordonwoodhull commented Mar 17, 2018 •

edited

Loading

gordonwoodhull Apr 2, 2018 •

edited

Loading

Justus-Maier commented Apr 3, 2018 •

edited

Loading

gordonwoodhull commented Apr 3, 2018 •

edited

Loading