Merge pull request #2785 from plotly/box-violin-innerpart-removal

etpinard · web-flow · commit 7e1c98cff271 · 2018-07-06T18:50:38.000-04:00
Fix box &amp; violin inner parts removal
diff --git a/src/traces/box/plot.js b/src/traces/box/plot.js
@@ -73,18 +73,9 @@ function plot(gd, plotinfo, cdbox, boxLayer) {
         // always split the distance to the closest box
         t.wHover = t.dPos * (group ? groupFraction / numBoxes : 1);
 
-        // boxes and whiskers
         plotBoxAndWhiskers(sel, {pos: posAxis, val: valAxis}, trace, t);
-
-        // draw points, if desired
-        if(trace.boxpoints) {
-            plotPoints(sel, {x: xa, y: ya}, trace, t);
-        }
-
-        // draw mean (and stdev diamond) if desired
-        if(trace.boxmean) {
-            plotBoxMean(sel, {pos: posAxis, val: valAxis}, trace, t);
-        }
+        plotPoints(sel, {x: xa, y: ya}, trace, t);
+        plotBoxMean(sel, {pos: posAxis, val: valAxis}, trace, t);
     });
 }
 
@@ -109,7 +100,10 @@ function plotBoxAndWhiskers(sel, axes, trace, t) {
         bdPos1 = t.bdPos;
     }
 
-    var paths = sel.selectAll('path.box').data(Lib.identity);
+    var paths = sel.selectAll('path.box').data((
+        trace.type !== 'violin' ||
+        trace.box
+    ) ? Lib.identity : []);
 
     paths.enter().append('path')
         .style('vector-effect', 'non-scaling-stroke')
@@ -187,16 +181,18 @@ function plotPoints(sel, axes, trace, t) {
     // repeatable pseudo-random number generator
     Lib.seedPseudoRandom();
 
-    var gPoints = sel.selectAll('g.points')
-        // since box plot points get an extra level of nesting, each
-        // box needs the trace styling info
-        .data(function(d) {
-            d.forEach(function(v) {
-                v.t = t;
-                v.trace = trace;
-            });
-            return d;
+    // since box plot points get an extra level of nesting, each
+    // box needs the trace styling info
+    var fn = function(d) {
+        d.forEach(function(v) {
+            v.t = t;
+            v.trace = trace;
         });
+        return d;
+    };
+
+    var gPoints = sel.selectAll('g.points')
+        .data(mode ? fn : []);
 
     gPoints.enter().append('g')
         .attr('class', 'points');
@@ -292,6 +288,9 @@ function plotBoxMean(sel, axes, trace, t) {
     var bPos = t.bPos;
     var bPosPxOffset = t.bPosPxOffset || 0;
 
+    // to support violin mean lines
+    var mode = trace.boxmean || (trace.meanline || {}).visible;
+
     // to support for one-sided box
     var bdPos0;
     var bdPos1;
@@ -303,7 +302,10 @@ function plotBoxMean(sel, axes, trace, t) {
         bdPos1 = t.bdPos;
     }
 
-    var paths = sel.selectAll('path.mean').data(Lib.identity);
+    var paths = sel.selectAll('path.mean').data((
+        (trace.type === 'box' && trace.boxmean) ||
+        (trace.type === 'violin' && trace.box && trace.meanline)
+    ) ? Lib.identity : []);
 
     paths.enter().append('path')
         .attr('class', 'mean')
@@ -325,14 +327,14 @@ function plotBoxMean(sel, axes, trace, t) {
         if(trace.orientation === 'h') {
             d3.select(this).attr('d',
                 'M' + m + ',' + pos0 + 'V' + pos1 +
-                (trace.boxmean === 'sd' ?
+                (mode === 'sd' ?
                     'm0,0L' + sl + ',' + posc + 'L' + m + ',' + pos0 + 'L' + sh + ',' + posc + 'Z' :
                     '')
             );
         } else {
             d3.select(this).attr('d',
                 'M' + pos0 + ',' + m + 'H' + pos1 +
-                (trace.boxmean === 'sd' ?
+                (mode === 'sd' ?
                     'm0,0L' + posc + ',' + sl + 'L' + pos0 + ',' + m + 'L' + posc + ',' + sh + 'Z' :
                     '')
             );
diff --git a/src/traces/violin/plot.js b/src/traces/violin/plot.js
@@ -69,8 +69,6 @@ module.exports = function plot(gd, plotinfo, cd, violinLayer) {
         var hasBothSides = trace.side === 'both';
         var hasPositiveSide = hasBothSides || trace.side === 'positive';
         var hasNegativeSide = hasBothSides || trace.side === 'negative';
-        var hasBox = trace.box && trace.box.visible;
-        var hasMeanLine = trace.meanline && trace.meanline.visible;
         var groupStats = fullLayout._violinScaleGroupStats[trace.scalegroup];
 
         var violins = sel.selectAll('path.violin').data(Lib.identity);
@@ -149,66 +147,61 @@ module.exports = function plot(gd, plotinfo, cd, violinLayer) {
             d.pathLength = d.path.getTotalLength() / (hasBothSides ? 2 : 1);
         });
 
-        if(hasBox) {
-            var boxWidth = trace.box.width;
-            var boxLineWidth = trace.box.line.width;
-            var bdPosScaled;
-            var bPosPxOffset;
+        var boxAttrs = trace.box || {};
+        var boxWidth = boxAttrs.width;
+        var boxLineWidth = (boxAttrs.line || {}).width;
+        var bdPosScaled;
+        var bPosPxOffset;
+
+        if(hasBothSides) {
+            bdPosScaled = bdPos * boxWidth;
+            bPosPxOffset = 0;
+        } else if(hasPositiveSide) {
+            bdPosScaled = [0, bdPos * boxWidth / 2];
+            bPosPxOffset = -boxLineWidth;
+        } else {
+            bdPosScaled = [bdPos * boxWidth / 2, 0];
+            bPosPxOffset = boxLineWidth;
+        }
 
-            if(hasBothSides) {
-                bdPosScaled = bdPos * boxWidth;
-                bPosPxOffset = 0;
-            } else if(hasPositiveSide) {
-                bdPosScaled = [0, bdPos * boxWidth / 2];
-                bPosPxOffset = -boxLineWidth;
-            } else {
-                bdPosScaled = [bdPos * boxWidth / 2, 0];
-                bPosPxOffset = boxLineWidth;
-            }
+        // inner box
+        boxPlot.plotBoxAndWhiskers(sel, {pos: posAxis, val: valAxis}, trace, {
+            bPos: bPos,
+            bdPos: bdPosScaled,
+            bPosPxOffset: bPosPxOffset
+        });
 
-            boxPlot.plotBoxAndWhiskers(sel, {pos: posAxis, val: valAxis}, trace, {
-                bPos: bPos,
-                bdPos: bdPosScaled,
-                bPosPxOffset: bPosPxOffset
-            });
-
-            // if both box and meanline are visible, show mean line inside box
-            if(hasMeanLine) {
-                boxPlot.plotBoxMean(sel, {pos: posAxis, val: valAxis}, trace, {
-                    bPos: bPos,
-                    bdPos: bdPosScaled,
-                    bPosPxOffset: bPosPxOffset
-                });
-            }
-        }
-        else {
-            if(hasMeanLine) {
-                var meanPaths = sel.selectAll('path.mean').data(Lib.identity);
-
-                meanPaths.enter().append('path')
-                    .attr('class', 'mean')
-                    .style({
-                        fill: 'none',
-                        'vector-effect': 'non-scaling-stroke'
-                    });
-
-                meanPaths.exit().remove();
-
-                meanPaths.each(function(d) {
-                    var v = valAxis.c2p(d.mean, true);
-                    var p = helpers.getPositionOnKdePath(d, trace, v);
-
-                    d3.select(this).attr('d',
-                        trace.orientation === 'h' ?
-                            'M' + v + ',' + p[0] + 'V' + p[1] :
-                            'M' + p[0] + ',' + v + 'H' + p[1]
-                    );
-                });
-            }
-        }
+        // meanline insider box
+        boxPlot.plotBoxMean(sel, {pos: posAxis, val: valAxis}, trace, {
+            bPos: bPos,
+            bdPos: bdPosScaled,
+            bPosPxOffset: bPosPxOffset
+        });
 
-        if(trace.points) {
-            boxPlot.plotPoints(sel, {x: xa, y: ya}, trace, t);
+        var fn;
+        if(!(trace.box || {}).visible && (trace.meanline || {}).visible) {
+            fn = Lib.identity;
         }
+
+        // N.B. use different class name than boxPlot.plotBoxMean,
+        // to avoid selectAll conflict
+        var meanPaths = sel.selectAll('path.meanline').data(fn || []);
+        meanPaths.enter().append('path')
+            .attr('class', 'meanline')
+            .style('fill', 'none')
+            .style('vector-effect', 'non-scaling-stroke');
+        meanPaths.exit().remove();
+        meanPaths.each(function(d) {
+            var v = valAxis.c2p(d.mean, true);
+            var p = helpers.getPositionOnKdePath(d, trace, v);
+
+            d3.select(this).attr('d',
+                trace.orientation === 'h' ?
+                    'M' + v + ',' + p[0] + 'V' + p[1] :
+                    'M' + p[0] + ',' + v + 'H' + p[1]
+            );
+        });
+
+        boxPlot.plotPoints(sel, {x: xa, y: ya}, trace, t);
     });
 };
diff --git a/src/traces/violin/style.js b/src/traces/violin/style.js
@@ -35,11 +35,17 @@ module.exports = function style(gd, cd) {
             .call(Color.stroke, boxLine.color)
             .call(Color.fill, box.fillcolor);
 
+        var meanLineStyle = {
+            'stroke-width': meanLineWidth + 'px',
+            'stroke-dasharray': (2 * meanLineWidth) + 'px,' + meanLineWidth + 'px'
+        };
+
         sel.selectAll('path.mean')
-            .style({
-                'stroke-width': meanLineWidth + 'px',
-                'stroke-dasharray': (2 * meanLineWidth) + 'px,' + meanLineWidth + 'px'
-            })
+            .style(meanLineStyle)
+            .call(Color.stroke, meanline.color);
+
+        sel.selectAll('path.meanline')
+            .style(meanLineStyle)
             .call(Color.stroke, meanline.color);
 
         stylePoints(sel, trace, gd);
diff --git a/test/jasmine/tests/box_test.js b/test/jasmine/tests/box_test.js
@@ -3,6 +3,7 @@ var Lib = require('@src/lib');
 
 var Box = require('@src/traces/box');
 
+var d3 = require('d3');
 var createGraphDiv = require('../assets/create_graph_div');
 var destroyGraphDiv = require('../assets/destroy_graph_div');
 var failTest = require('../assets/fail_test');
@@ -348,3 +349,54 @@ describe('Box edge cases', function() {
         .then(done);
     });
 });
+
+describe('Test box restyle:', function() {
+    var gd;
+
+    beforeEach(function() {
+        gd = createGraphDiv();
+    });
+
+    afterEach(destroyGraphDiv);
+
+    it('should be able to add/remove innner parts', function(done) {
+        var fig = Lib.extendDeep({}, require('@mocks/box_plot_jitter.json'));
+        // start with just 1 box
+        delete fig.data[0].boxpoints;
+
+        function _assertOne(msg, exp, trace3, k, query) {
+            expect(trace3.selectAll(query).size())
+                .toBe(exp[k] || 0, k + ' - ' + msg);
+        }
+
+        function _assert(msg, exp) {
+            var trace3 = d3.select(gd).select('.boxlayer > .trace');
+            _assertOne(msg, exp, trace3, 'boxCnt', 'path.box');
+            _assertOne(msg, exp, trace3, 'meanlineCnt', 'path.mean');
+            _assertOne(msg, exp, trace3, 'ptsCnt', 'path.point');
+        }
+
+        Plotly.plot(gd, fig)
+        .then(function() {
+            _assert('base', {boxCnt: 1});
+        })
+        .then(function() { return Plotly.restyle(gd, 'boxmean', true); })
+        .then(function() {
+            _assert('with meanline', {boxCnt: 1, meanlineCnt: 1});
+        })
+        .then(function() { return Plotly.restyle(gd, 'boxmean', 'sd'); })
+        .then(function() {
+            _assert('with mean+sd line', {boxCnt: 1, meanlineCnt: 1});
+        })
+        .then(function() { return Plotly.restyle(gd, 'boxpoints', 'all'); })
+        .then(function() {
+            _assert('with mean+sd line + pts', {boxCnt: 1, meanlineCnt: 1, ptsCnt: 9});
+        })
+        .then(function() { return Plotly.restyle(gd, 'boxmean', false); })
+        .then(function() {
+            _assert('with pts', {boxCnt: 1, ptsCnt: 9});
+        })
+        .catch(failTest)
+        .then(done);
+    });
+});
diff --git a/test/jasmine/tests/violin_test.js b/test/jasmine/tests/violin_test.js
