Consider first review

archmoj · archmoj · commit a27dc29ca873 · 2019-12-13T16:01:51.000-05:00
- make pie insidetextorientation coerce condition more readable
- no need for extra check before recompute bounding box
- revise uniformtext attribute descriptions
- remove boolean from getTextTransform arguments
- add js doc description
- create and use Lib.ensureUniformFontSize
- make the hide/show logic independent of _module.plot
- refactor textposition coerce logic
- add test for inside-text-orientation coerce logic with various textposition scenarios
- use radial tangential and horizontal in inside-text-orientation keys
diff --git a/src/lib/index.js b/src/lib/index.js
@@ -1182,13 +1182,17 @@ lib.isHidden = function(gd) {
     return !display || display === 'none';
 };
 
-lib.getTextTransform = function(opts, isCenter) {
-    var textX = opts.textX;
-    var textY = opts.textY;
-    var targetX = opts.targetX;
-    var targetY = opts.targetY;
-    var rotate = opts.rotate;
-    var scale = opts.scale;
+/** Return transform text for bar bar-like rectangles and pie-like slices
+ *  @param {object} transform
+ */
+lib.getTextTransform = function(transform) {
+    var noCenter = transform.noCenter;
+    var textX = transform.textX;
+    var textY = transform.textY;
+    var targetX = transform.targetX;
+    var targetY = transform.targetY;
+    var rotate = transform.rotate;
+    var scale = transform.scale;
     if(!scale) scale = 0;
     else if(scale > 1) scale = 1;
 
@@ -1198,14 +1202,21 @@ lib.getTextTransform = function(opts, isCenter) {
             (targetY - scale * textY) +
         ')' +
         (scale < 1 ?
-            'scale(' + scale + ')' :
-            ''
+            'scale(' + scale + ')' : ''
         ) +
         (rotate ?
             'rotate(' + rotate +
-                (isCenter ? '' : ' ' + textX + ' ' + textY) +
-            ')' :
-            ''
+                (noCenter ? '' : ' ' + textX + ' ' + textY) +
+            ')' : ''
         )
     );
 };
+
+lib.ensureUniformFontSize = function(gd, baseFont) {
+    var out = lib.extendFlat({}, baseFont);
+    out.size = Math.max(
+        baseFont.size,
+        gd._fullLayout.uniformtext.minsize || 0
+    );
+    return out;
+};
diff --git a/src/plots/layout_attributes.js b/src/plots/layout_attributes.js
@@ -151,11 +151,11 @@ module.exports = {
                 'Determines how the font size for various text',
                 'elements are uniformed between each trace type.',
                 'If the computed text sizes were smaller than',
-                'the minimum size defined by `minsize`',
+                'the minimum size defined by `uniformtext.minsize`',
                 'using *hide* option hides the text; and',
                 'using *show* option shows the text without further downscaling.',
                 'Please note that if the size defined by `minsize` is greater than',
-                'the font size defined by trace, the `minsize` would be used.'
+                'the font size defined by trace, then the `minsize` is used.'
             ].join(' ')
         },
         minsize: {
diff --git a/src/traces/bar/plot.js b/src/traces/bar/plot.js
@@ -329,8 +329,7 @@ function appendBarText(gd, plotinfo, bar, cd, i, x0, x1, y0, y1, opts, makeOnCom
             // draw text using insideTextFont and check if it fits inside bar
             textPosition = 'inside';
 
-            font = Lib.extendFlat({}, insideTextFont, {});
-            font.size = Math.max(font.size, fullLayout.uniformtext.minsize || 0);
+            font = Lib.ensureUniformFontSize(gd, insideTextFont);
 
             textSelection = appendTextNode(bar, text, font);
 
@@ -362,8 +361,7 @@ function appendBarText(gd, plotinfo, bar, cd, i, x0, x1, y0, y1, opts, makeOnCom
     }
 
     if(!textSelection) {
-        font = Lib.extendFlat({}, (textPosition === 'outside') ? outsideTextFont : insideTextFont, {});
-        font.size = Math.max(font.size, fullLayout.uniformtext.minsize || 0);
+        font = Lib.ensureUniformFontSize(gd, (textPosition === 'outside') ? outsideTextFont : insideTextFont);
 
         textSelection = appendTextNode(bar, text, font);
 
diff --git a/src/traces/bar/style.js b/src/traces/bar/style.js
@@ -38,20 +38,11 @@ function resizeText(gd, gTrace, traceType) {
                 t = gTrace.selectAll('g.points').selectAll('g.point').selectAll('text');
         }
 
-        var isCenter = (
-            traceType === 'pie' ||
-            traceType === 'sunburst'
-        );
-
         t.each(function(d) {
             var transform = d.transform;
-
             transform.scale = minSize / transform.fontSize;
-            d3.select(this).attr('transform', Lib.getTextTransform(transform, isCenter));
-
-            if(shouldHide && transform.hide) {
-                d3.select(this).text(' ');
-            }
+            d3.select(this).attr('transform', Lib.getTextTransform(transform));
+            d3.select(this).attr('display', shouldHide && transform.hide ? 'none' : null);
         });
     }
 }
@@ -95,8 +86,7 @@ function stylePoints(sel, trace, gd) {
 function styleTextPoints(sel, trace, gd) {
     sel.selectAll('text').each(function(d) {
         var tx = d3.select(this);
-        var font = Lib.extendFlat({}, determineFont(tx, d, trace, gd), {});
-        font.size = Math.max(font.size, gd._fullLayout.uniformtext.minsize || 0);
+        var font = Lib.ensureUniformFontSize(gd, determineFont(tx, d, trace, gd));
 
         Drawing.font(tx, font);
     });
@@ -124,8 +114,7 @@ function styleTextInSelectionMode(txs, trace, gd) {
         var font;
 
         if(d.selected) {
-            font = Lib.extendFlat({}, determineFont(tx, d, trace, gd));
-            font.size = Math.max(font.size, gd._fullLayout.uniformtext.minsize || 0);
+            font = Lib.ensureUniformFontSize(gd, determineFont(tx, d, trace, gd));
 
             var selectedFontColor = trace.selected.textfont && trace.selected.textfont.color;
             if(selectedFontColor) {
diff --git a/src/traces/funnelarea/plot.js b/src/traces/funnelarea/plot.js
@@ -96,8 +96,7 @@ module.exports = function plot(gd, cdModule) {
                         s.attr('data-notex', 1);
                     });
 
-                    var font = Lib.extendFlat({}, determineInsideTextFont(trace, pt, fullLayout.font), {});
-                    font.size = Math.max(font.size, fullLayout.uniformtext.minsize || 0);
+                    var font = Lib.ensureUniformFontSize(gd, determineInsideTextFont(trace, pt, fullLayout.font));
 
                     sliceText.text(pt.text)
                         .attr({
diff --git a/src/traces/pie/attributes.js b/src/traces/pie/attributes.js
@@ -184,16 +184,16 @@ module.exports = {
     insidetextorientation: {
         valType: 'enumerated',
         role: 'info',
-        values: ['h', 'r', 't', 'auto'],
+        values: ['horizontal', 'radial', 'tangential', 'auto'],
         dflt: 'auto',
         editType: 'plot',
         description: [
             'Determines the orientation of text inside slices.',
             'With *auto* the texts may automatically be',
             'rotated to fit with the maximum size inside the slice.',
-            'Using *h* option forces text to be horizontal.',
-            'Using *r* option forces text to be radial.',
-            'Using *t* option forces text to be tangential.'
+            'Using *horizontal* option forces text to be horizontal.',
+            'Using *radial* option forces text to be radial.',
+            'Using *tangential* option forces text to be tangential.'
         ].join(' ')
     },
     insidetextfont: extendFlat({}, textFontAttrs, {
diff --git a/src/traces/pie/defaults.js b/src/traces/pie/defaults.js
@@ -71,7 +71,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
             coerce('automargin');
         }
 
-        if(textposition !== 'none' && textposition !== 'outside') {
+        if(textposition === 'inside' || textposition === 'auto' || Array.isArray(textposition)) {
             coerce('insidetextorientation');
         }
     }
diff --git a/src/traces/pie/plot.js b/src/traces/pie/plot.js
@@ -145,11 +145,10 @@ function plot(gd, cdModule) {
                         s.attr('data-notex', 1);
                     });
 
-                    var font = Lib.extendFlat({}, textPosition === 'outside' ?
+                    var font = Lib.ensureUniformFontSize(gd, textPosition === 'outside' ?
                         determineOutsideTextFont(trace, pt, fullLayout.font) :
-                        determineInsideTextFont(trace, pt, fullLayout.font), {}
+                        determineInsideTextFont(trace, pt, fullLayout.font)
                     );
-                    font.size = Math.max(font.size, fullLayout.uniformtext.minsize || 0);
 
                     sliceText.text(pt.text)
                         .attr({
@@ -169,14 +168,11 @@ function plot(gd, cdModule) {
                     } else {
                         transform = transformInsideText(textBB, pt, cd0);
                         if(textPosition === 'auto' && transform.scale < 1) {
-                            var newFont = Lib.extendFlat({}, trace.outsidetextfont, {});
-                            newFont.size = Math.max(newFont.size, fullLayout.uniformtext.minsize || 0);
+                            var newFont = Lib.ensureUniformFontSize(gd, trace.outsidetextfont);
 
                             sliceText.call(Drawing.font, newFont);
-                            if(newFont.family !== font.family || newFont.size !== font.size) {
-                                // recompute bounding box
-                                textBB = Drawing.bBox(sliceText.node());
-                            }
+                            textBB = Drawing.bBox(sliceText.node());
+
                             transform = transformOutsideText(textBB, pt);
                         }
                     }
@@ -201,7 +197,7 @@ function plot(gd, cdModule) {
                     recordMinTextSize(trace.type, transform, fullLayout);
                     cd[i].transform = transform;
 
-                    sliceText.attr('transform', Lib.getTextTransform(transform, true));
+                    sliceText.attr('transform', Lib.getTextTransform(transform));
                 });
             });
 
@@ -565,11 +561,14 @@ function transformInsideText(textBB, pt, cd0) {
     var rInscribed = pt.rInscribed;
     var r = cd0.r || pt.rpx1;
     var orientation = cd0.trace.insidetextorientation;
-    var allTransforms = [];
-
+    var isHorizontal = orientation === 'horizontal';
+    var isTangential = orientation === 'tangential';
+    var isRadial = orientation === 'radial';
+    var isAuto = orientation === 'auto';
     var isCircle = (ring === 1) && (Math.abs(pt.startangle - pt.stopangle) === Math.PI * 2);
+    var allTransforms = [];
 
-    if(isCircle || orientation === 'auto' || orientation === 'h') {
+    if(isCircle || isAuto || isHorizontal) {
         // max size text can be inserted inside without rotating it
         // this inscribes the text rectangle in a circle, which is then inscribed
         // in the slice, so it will be an underestimate, which some day we may want
@@ -587,7 +586,7 @@ function transformInsideText(textBB, pt, cd0) {
         allTransforms.push(transform);
     }
 
-    if(orientation === 'h') {
+    if(isHorizontal) {
         // max size if text is placed (horizontally) at the top or bottom of the arc
 
         var considerCrossing = function(angle, key) {
@@ -603,32 +602,33 @@ function transformInsideText(textBB, pt, cd0) {
                 } else { // case of 'rad'
                     newT = calcRadTransform(textBB, r, ring, closestEdge, Math.PI / 2);
                 }
-                newT._repos = getCoords(r, angle);
+                newT.pxtxt = getCoords(r, angle);
 
                 allTransforms.push(newT);
             }
         };
 
         for(var i = 3; i >= -3; i--) { // to cover all cases with trace.rotation added
-            considerCrossing(Math.PI * (i + 0.0), 'tan');
+            considerCrossing(Math.PI * i, 'tan');
             considerCrossing(Math.PI * (i + 0.5), 'rad');
         }
     }
 
-    if(orientation === 'auto' || orientation === 'r') {
+    if(isAuto || isRadial) {
         allTransforms.push(calcRadTransform(textBB, r, ring, halfAngle, midAngle));
     }
 
-    if(orientation === 'auto' || orientation === 't') {
+    if(isAuto || isTangential) {
         allTransforms.push(calcTanTransform(textBB, r, ring, halfAngle, midAngle));
     }
 
     var maxScaleTransform = allTransforms.sort(function(a, b) {
         return b.scale - a.scale;
     })[0];
 
-    if(maxScaleTransform._repos) {
-        pt.pxtxt = maxScaleTransform._repos;
+    if(maxScaleTransform.pxtxt) {
+        // copy text position if not at the middle
+        pt.pxtxt = maxScaleTransform.pxtxt;
     }
 
     return maxScaleTransform;
@@ -1119,6 +1119,7 @@ function computeTransform(
     var midY = (textBB.top + textBB.bottom) / 2;
     transform.textX = midX * cosA - midY * sinA;
     transform.textY = midX * sinA + midY * cosA;
+    transform.noCenter = true;
 }
 
 module.exports = {
diff --git a/src/traces/sunburst/plot.js b/src/traces/sunburst/plot.js
@@ -253,8 +253,7 @@ function plotOne(gd, cd, element, transitionOpts) {
             s.attr('data-notex', 1);
         });
 
-        var font = Lib.extendFlat({}, helpers.determineTextFont(trace, pt, fullLayout.font), {});
-        font.size = Math.max(font.size, fullLayout.uniformtext.minsize || 0);
+        var font = Lib.ensureUniformFontSize(gd, helpers.determineTextFont(trace, pt, fullLayout.font));
 
         sliceText.text(exports.formatSliceLabel(pt, entry, trace, cd, fullLayout))
             .classed('slicetext', true)
diff --git a/src/traces/treemap/draw_ancestors.js b/src/traces/treemap/draw_ancestors.js
@@ -141,8 +141,7 @@ module.exports = function drawAncestors(gd, cd, entry, slices, opts) {
             s.attr('data-notex', 1);
         });
 
-        var font = Lib.extendFlat({}, helpers.determineTextFont(trace, pt, fullLayout.font, trace.pathdir), {});
-        font.size = Math.max(font.size, fullLayout.uniformtext.minsize || 0);
+        var font = Lib.ensureUniformFontSize(gd, helpers.determineTextFont(trace, pt, fullLayout.font, trace.pathdir));
 
         sliceText.text(pt._text || ' ') // use one space character instead of a blank string to avoid jumps during transition
             .classed('slicetext', true)
diff --git a/src/traces/treemap/draw_descendants.js b/src/traces/treemap/draw_descendants.js
@@ -182,8 +182,7 @@ module.exports = function drawDescendants(gd, cd, entry, slices, opts) {
             s.attr('data-notex', 1);
         });
 
-        var font = Lib.extendFlat({}, helpers.determineTextFont(trace, pt, fullLayout.font), {});
-        font.size = Math.max(font.size, fullLayout.uniformtext.minsize || 0);
+        var font = Lib.ensureUniformFontSize(gd, helpers.determineTextFont(trace, pt, fullLayout.font));
 
         sliceText.text(pt._text || ' ') // use one space character instead of a blank string to avoid jumps during transition
             .classed('slicetext', true)
diff --git a/test/image/mocks/pie_inside-text-orientation.json b/test/image/mocks/pie_inside-text-orientation.json
@@ -8,7 +8,7 @@
                     "size": 20
                 }
             },
-            "insidetextorientation": "h",
+            "insidetextorientation": "horizontal",
             "textposition": "inside",
             "type": "pie",
             "hole": 0.5,
@@ -117,7 +117,7 @@
                     "size": 20
                 }
             },
-            "insidetextorientation": "r",
+            "insidetextorientation": "radial",
             "textposition": "inside",
             "type": "pie",
             "hole": 0.5,
@@ -197,7 +197,7 @@
                     "size": 20
                 }
             },
-            "insidetextorientation": "t",
+            "insidetextorientation": "tangential",
             "textposition": "inside",
             "type": "pie",
             "hole": 0.5,
diff --git a/test/image/mocks/sunburst_inside-text-orientation.json b/test/image/mocks/sunburst_inside-text-orientation.json
@@ -2,7 +2,7 @@
     "data": [
         {
             "name": "horizontal",
-            "insidetextorientation": "h",
+            "insidetextorientation": "horizontal",
             "type": "sunburst",
             "level": "Juliet",
             "parents": [
@@ -105,7 +105,7 @@
         },
         {
             "name": "radial",
-            "insidetextorientation": "r",
+            "insidetextorientation": "radial",
             "type": "sunburst",
             "level": "Juliet",
             "parents": [
@@ -179,7 +179,7 @@
         },
         {
             "name": "tangential",
-            "insidetextorientation": "t",
+            "insidetextorientation": "tangential",
             "type": "sunburst",
             "level": "Juliet",
             "parents": [
diff --git a/test/image/mocks/uniformtext_pie_8_horizontal.json b/test/image/mocks/uniformtext_pie_8_horizontal.json
diff --git a/test/image/mocks/uniformtext_pie_8_horizontal_center.json b/test/image/mocks/uniformtext_pie_8_horizontal_center.json
diff --git a/test/image/mocks/uniformtext_pie_pull.json b/test/image/mocks/uniformtext_pie_pull.json
diff --git a/test/image/mocks/uniformtext_sunburst_treemap.json b/test/image/mocks/uniformtext_sunburst_treemap.json
diff --git a/test/jasmine/tests/pie_test.js b/test/jasmine/tests/pie_test.js

Original file line number	Diff line number	Diff line change
`@@ -71,7 +71,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout`
`71`	`71`	`coerce('automargin');`
`72`	`72`	`}`
`73`	`73`
`74`		`- if(textposition !== 'none' && textposition !== 'outside') {`
	`74`	`+ if(textposition === 'inside' \|\| textposition === 'auto' \|\| Array.isArray(textposition)) {`
`75`	`75`	`coerce('insidetextorientation');`
`76`	`76`	`}`
`77`	`77`	`}`