d3v4 cleanup

[gordonwoodhull]

Hi @kum-deepak!

As promised, I'm doing a thorough line-by line review of #1363 d3v4 support.

Rather than comment inline on a PR that has already been merged, I decided to start a new issue.

I think I have gotten through about 1/3 of the changes, and I'm learning a lot along the way!

Here are my comments so far. I'm going to go for a Saturday walk now. :)

global changes

"selection"

The argument name selection is confusing for fadeDeselectedArea/brushIsEmpty/extendBrush / scatterPlot.brushIsEmpty scatterPlot._brushing because it sounds like it should be a d3 selection. Personally I would use the term filter or extent here.

I guess this arose because of event.selection from Brush Events so maybe brushSelection would be a good compromise. Kind of confusing that they reused such a common term.

composite chart

rightYAxis documentation: "depending on whether you are going to use the axis on left or right" doesn't make sense, as it should always be d3.axisRight. I guess this was copied from coordinateGridMixin.yAxis but it's different.

coordinate grid mixin

xUnitCount

This function now distinguishes dc.units.ordinal but it doesn't need to because calling the function would have exactly the same effect. I'd rather not add the special case unless it's necessary for some reason? The design of xUnits functions is perhaps a bit strange but it is generic.

If this really needs to be then it should at least use chart.isOrdinal().

prepareXAxis

I agree that we need to use d3.scaleBand instead of d3.scaleOrdinal -- it's so much cleaner! -- but it will confuse users if their input .x() scale is changed. We should issue a warning every time we make the replacement so users can update their code accordingly.

prepareYAxis

       // Ideally we should update the API so that if someone uses Right Y Axis
       // they would need to pass _yAxis as well
       if (!_yAxis) {
           if (_useRightYAxis) {
                _yAxis = d3.axisRight();
            } else {
                _yAxis = d3.axisLeft();
            }
         }

I respectfully disagree with the comment - it's really nice how the axes are automatically initialized and I think we should be able to extend that to the right axis.

This code will never get hit because _yAxis is initialized and the user would never set it to undefined or null.

Could we start with _yAxis null and initialize it like we're doing here, when

• this function is hit, or
• when .yAxis() is called as a getter?

I think this would safely initialize it to axisLeft or axisRight automatically, and it would be nice not to default it wrong. The only way it might go wrong is if the user called .yAxis() before calling .useRightAxis(true) but I doubt anyone would do that.

Also, I know it was already there in the original, but I find

        _yAxis = _yAxis.scale(_y);

really confusing. .scale() is an ordinary setter which modifies the axis.

[kum-deepak]

👍 I agree to all of these, I will work on these when I am through with the brush and zoom :)

After having worked for about 2 months now, I understand that it will make everyone's life easier if getter/setters did not do anything other than updating the underlying variable. Newer code I have started following that. When we are in more control with the upgrade, If you find it useful, we can make a pass to cleanup the entire code base in this regard.

[gordonwoodhull]

I mostly agree, but yAxis is one exception to the rule, and I think I've seen other cases like this one. Here there is implicitly a default but we don't actually want to create it until it's asked for, because it depends on another variable.

By all means, wherever we can remove extra code from getter/setters without changing the behavior, we should do that. Often that code will drift into prepare/initialize. I don't know if this is always possible.

[gordonwoodhull]

Here's part 2. Also filed a few issues. Going to get some weekend rest now. :)

coordinate grid mixin

(continued)

_renderHorizontalGridLinesForAxis

Thanks for fixing
scale.ticks(axis.ticks()[0]) => scale.ticks.apply(scale, axis.tickArguments())!
I'm going ahead and fixing renderVerticalGridLines the same way. I'm also adding it to Changelog.md (under 3.0 alpha 1) - you can help me out by recording any interface changes there too.

brushWidth, brushHeight

I don't see the point of having these functions, since they will always be _chart.effectiveWidth(), _chart.effectiveHeight(). Looks like brushHeight was created before effectiveHeight existed (and was always incorrect).

Were you running into bugs with the brush height? Should we document the change?

handle--custom

Odd class name - I think we usually do something like dc-custom-handle.

[kum-deepak]

Agree to part 2 list as well.

[gordonwoodhull]

Okay, here's the final part 3/3 of my review. I've also made some minor edits and additions above.

Truly a heroic effort you made here. I've learned so much reading your changes. Thank You!

number display

d3.easeQuadIn: I think the intent here was d3.easeQuadInOut (aka d3.easeQuad). The previous quad-out-in looks wrong to me (starts fast then slow in the middle then fast again). It's trivial but we should note this in changelog.

pie chart

createSliceNodes / createElements / drawChart

It looks like you went to some trouble to return arrays with multiple values, and then unpack the arrays, with the comment "Uglify does not like array assignments".

Do you mean it doesn't like destructuring assignment? If so, that's because DA is an ES6 feature, and unfortunately we are still stuck in ES5. (I can't wait till we can drop IE but I'm not willing to transpile.)

But most of those return values are unused - slices and labels in drawCharts, thus slices and labels in createElements, thus labels in createLabels... Unless I'm missing something?

I'd rather keep it simple, just return what is used, and drop the confusing comments. Does that make sense?

row chart

(I am starting to see what you mean about simplifying the code by reordering exit/enter/update #1380. But I think it would mean squashing all the draws into one function and that's a huge change. I'm sure it could be done safely, but still, huge.)

scatter plot

brushIsEmpty

The old >= seems safer to me than === - do we know that selection will always be valid?

_brushing

// Testing with pixels is more reliable

I get that you want to test in pixel coordinates rather than data coordinates, and I agree that's often safer. However, I think this misses one case (which apparently we don't have tests for).

Previously we checked if the brush was empty after extendBrush, which really rounds the brush. (Maybe it should be called roundBrush but it would be difficult to change now.)

So this misses the case where rounding is enabled and you have dragged only a little way. Previously this would round to no extent and result in no filter, but now it will apply a filter which will match nothing. You can see this in action in the scatter-brushing example if you add

    .round(Math.floor)

Not a big deal, not sure if anyone will notice, but it helps with sloppy clicks, which can look like short drags. And it means that if no brush is shown then there is no filter.

Were you running into precision problems comparing the data coordinates?

select menu

        // indicate that no one should use return value
        return null;

IMO it's fine just not to return anything - it's normal for JS functions to implicitly return undefined.

utils

Good work documenting the odd behavior of add and subtract.

[kum-deepak]

Thanks for your detailed review :)

I will come back to changes needed for coordinate grid mixin after the brush related changes are merged.

Meanwhile I will cover other remarks.

[kum-deepak]

A list for me to track (tick indicates completed or PR raised):

• Global - "selection"
• composite chart - Right Y Axis
• coordinate grid mixin - xUnitCount
• coordinate grid mixin - prepareXAxis
• coordinate grid mixin - prepareYAxis
• coordinate grid mixin - _renderHorizontalGridLinesForAxis
• coordinate grid mixin - brushWidth, brushHeight
• coordinate grid mixin - handle--custom
• number display
• pie chart
• row chart
• scatter plot - brushIsEmpty
• scatter plot - _brushing
• select menu
• utils

[kum-deepak]

As of now skipping any changes in row-chart. At this stage, I think, we should keep the change-set small in comparison to v2.1.

Once we are through with v3 release, we can discuss what all we can take up.

[gordonwoodhull]

I think this is the only remaining change before 3.0 goes beta!

[kum-deepak]

Just pushed tho PRs, we should be able to close this issue now 😃

[gordonwoodhull]

Fixed in 3.0 beta 1

[Fil]

I've tried to use it in @observablehq with d3v5, and it sort of works, but some styling is off.
Also I could't require("dc"), I had to load it through bundle.run.

But other than that, it looks good.
https://beta.observablehq.com/@fil/hello-dc-js

Congratulations! And what an achievement!

[gordonwoodhull]

Thanks @Fil!

Yes, we'll have to iterate on it. I'm new to ES6 modules so I'm not surprised that we need to configure the module better. It's still just straight old UMD.

As for styling, I see two problems. One, the cell heights are too short, at least on my iPad. That is perhaps a CSS problem.

Two, you need to set xUnits in order to get the bar widths right. I wish dc.js could figure that out automatically, but it can't currently.

Yes, it was a huge amount of work, mostly by @kum-deepak. I'm really impressed that he figured this all out, and happy dc.js will live on for another few years!