From 21ad2cf2dbd1d63270801a5f69b87572c49e7ca7 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Tue, 11 Aug 2015 12:59:45 +0300 Subject: [PATCH 01/49] Remove height restriction for showing bar labels --- plottable.js | 2 +- src/plots/barPlot.ts | 2 +- test/plots/barPlotTests.ts | 12 +++++++----- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/plottable.js b/plottable.js index c444f52e3b..00bf60bea9 100644 --- a/plottable.js +++ b/plottable.js @@ -7918,7 +7918,7 @@ var Plottable; var secondaryAttrTextSpace = _this._isVertical ? measurement.width : measurement.height; var secondaryAttrAvailableSpace = _this._isVertical ? w : h; var tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - if (measurement.height <= h && measurement.width <= w) { + if (measurement.width <= w) { var offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { offset = offset * -1; diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 39bbac3e45..833ab5c754 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -431,7 +431,7 @@ export module Plots { let secondaryAttrTextSpace = this._isVertical ? measurement.width : measurement.height; let secondaryAttrAvailableSpace = this._isVertical ? w : h; let tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - if (measurement.height <= h && measurement.width <= w) { + if (measurement.width <= w) { let offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { offset = offset * -1; } if (this._isVertical) { diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index d3a21e675e..6f25e32b2f 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -695,9 +695,10 @@ describe("Plots", () => { plot.renderTo(svg); plot.labelsEnabled(true); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 2, "both texts drawn"); + assert.lengthOf(texts, 3, "all texts drawn"); assert.strictEqual(texts[0], "640", "first label is 640"); assert.strictEqual(texts[1], "12345", "first label is 12345"); + assert.strictEqual(texts[2], "5", "first label is 5"); svg.remove(); }); @@ -715,9 +716,10 @@ describe("Plots", () => { plot.labelFormatter((n: number) => n.toString() + "%"); plot.renderTo(svg); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 2, "both texts drawn"); - assert.strictEqual(texts[0], "640%", "first label is 640%"); - assert.strictEqual(texts[1], "12345%", "first label is 12345%"); + assert.lengthOf(texts, 3, "all texts drawn"); + assert.strictEqual(texts[0], "5%", "first label is 5%"); + assert.strictEqual(texts[1], "640%", "first label is 640%"); + assert.strictEqual(texts[2], "12345%", "first label is 12345%"); svg.remove(); }); @@ -725,7 +727,7 @@ describe("Plots", () => { plot.labelsEnabled(true); plot.renderTo(svg); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 2, "both texts drawn"); + assert.lengthOf(texts, 3, "all texts drawn"); let originalDrawLabels = ( plot)._drawLabels; let called = false; ( plot)._drawLabels = () => { From 54aaed01d227bad8e59eff937439d8aaff4ddbfa Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Tue, 11 Aug 2015 16:13:37 +0300 Subject: [PATCH 02/49] Set label colour based on position above or within bar --- plottable.css | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plottable.css b/plottable.css index 37475fc024..9ccc71364a 100644 --- a/plottable.css +++ b/plottable.css @@ -70,12 +70,12 @@ svg.plottable { font-size: 14px; } -.plottable .light-label text { - fill: white; +.plottable .above-bar-label text { + fill: #5279c7; } -.plottable .dark-label text { - fill: #32313F; +.plottable .in-bar-label text { + fill: white; } .plottable .axis-label text { From faec672aa49205ec587341140d19da38aa0a1ff1 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Tue, 11 Aug 2015 16:21:15 +0300 Subject: [PATCH 03/49] Show label above bar if it cannot fit within it --- plottable.js | 22 +++++++++++++--------- src/plots/barPlot.ts | 23 +++++++++++++---------- 2 files changed, 26 insertions(+), 19 deletions(-) diff --git a/plottable.js b/plottable.js index 00bf60bea9..81f3022978 100644 --- a/plottable.js +++ b/plottable.js @@ -7918,6 +7918,7 @@ var Plottable; var secondaryAttrTextSpace = _this._isVertical ? measurement.width : measurement.height; var secondaryAttrAvailableSpace = _this._isVertical ? w : h; var tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; + var showLabelAboveBar = (measurement.height > h); if (measurement.width <= w) { var offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { @@ -7929,11 +7930,18 @@ var Plottable; else { x += offset; } - var showLabel = true; var labelPosition = { x: x, - y: positive ? y : y + h - measurement.height + y: y }; + if (showLabelAboveBar) { + y = y - h + offset; + labelPosition.y = y; + } + else { + labelPosition.y = positive ? y : y + h - measurement.height; + } + ; if (_this._isVertical) { labelPosition.x = x + w / 2 - measurement.width / 2; } @@ -7945,14 +7953,10 @@ var Plottable; labelPosition.x = x; } } - if (labelPosition.x < 0 || labelPosition.x + measurement.width > _this.width() || - labelPosition.y < 0 || labelPosition.y + measurement.height > _this.height()) { - showLabel = false; - } var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - var className = dark ? "dark-label" : "light-label"; - g.classed(className, true); - g.style("visibility", showLabel ? "inherit" : "hidden"); + var labelPositioningClassName = showLabelAboveBar ? "above-bar-label" : "in-bar-label"; + g.classed(labelPositioningClassName, true); + g.style("visibility", "inherit"); var xAlign; var yAlign; if (_this._isVertical) { diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 833ab5c754..19099b0409 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -431,6 +431,7 @@ export module Plots { let secondaryAttrTextSpace = this._isVertical ? measurement.width : measurement.height; let secondaryAttrAvailableSpace = this._isVertical ? w : h; let tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; + let showLabelAboveBar = (measurement.height > h); if (measurement.width <= w) { let offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { offset = offset * -1; } @@ -440,10 +441,16 @@ export module Plots { x += offset; } - let showLabel = true; let labelPosition = { x: x, - y: positive ? y : y + h - measurement.height + y: y + }; + + if(showLabelAboveBar) { + y = y - h + offset; + labelPosition.y = y; + } else { + labelPosition.y = positive ? y : y + h - measurement.height; }; if (this._isVertical) { @@ -456,15 +463,11 @@ export module Plots { } } - if (labelPosition.x < 0 || labelPosition.x + measurement.width > this.width() || - labelPosition.y < 0 || labelPosition.y + measurement.height > this.height()) { - showLabel = false; - } - let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - let className = dark ? "dark-label" : "light-label"; - g.classed(className, true); - g.style("visibility", showLabel ? "inherit" : "hidden"); + let labelPositioningClassName = showLabelAboveBar ? "above-bar-label" : "in-bar-label"; + g.classed(labelPositioningClassName, true); + + g.style("visibility", "inherit"); let xAlign: string; let yAlign: string; if (this._isVertical) { From 75cc041b5846f0879d93a0156286a48727769fcb Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Tue, 11 Aug 2015 16:22:34 +0300 Subject: [PATCH 04/49] Filter out zeroes from data to draw --- plottable.js | 4 +++- src/plots/barPlot.ts | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/plottable.js b/plottable.js index 81f3022978..58d68a99de 100644 --- a/plottable.js +++ b/plottable.js @@ -8112,11 +8112,13 @@ var Plottable; Bar.prototype._getDataToDraw = function () { var dataToDraw = new Plottable.Utils.Map(); var attrToProjector = this._generateAttrToProjector(); + var primaryAccessor = this._isVertical ? this.y().accessor : this.x().accessor; this.datasets().forEach(function (dataset) { var data = dataset.data().filter(function (d, i) { return Plottable.Utils.Math.isValidNumber(attrToProjector["x"](d, i, dataset)) && Plottable.Utils.Math.isValidNumber(attrToProjector["y"](d, i, dataset)) && Plottable.Utils.Math.isValidNumber(attrToProjector["width"](d, i, dataset)) && - Plottable.Utils.Math.isValidNumber(attrToProjector["height"](d, i, dataset)); }); + Plottable.Utils.Math.isValidNumber(attrToProjector["height"](d, i, dataset)) && + (primaryAccessor(d, i, dataset) != 0); }); dataToDraw.set(dataset, data); }); return dataToDraw; diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 19099b0409..fd019b0724 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -634,11 +634,13 @@ export module Plots { protected _getDataToDraw() { let dataToDraw = new Utils.Map(); let attrToProjector = this._generateAttrToProjector(); + let primaryAccessor = this._isVertical ? this.y().accessor : this.x().accessor; this.datasets().forEach((dataset: Dataset) => { let data = dataset.data().filter((d, i) => Utils.Math.isValidNumber(attrToProjector["x"](d, i, dataset)) && Utils.Math.isValidNumber(attrToProjector["y"](d, i, dataset)) && Utils.Math.isValidNumber(attrToProjector["width"](d, i, dataset)) && - Utils.Math.isValidNumber(attrToProjector["height"](d, i, dataset))); + Utils.Math.isValidNumber(attrToProjector["height"](d, i, dataset)) && + (primaryAccessor(d, i, dataset) != 0)); dataToDraw.set(dataset, data); }); return dataToDraw; From 945a8934f40afa4828a61fe68510bdd42ca9e484 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Tue, 11 Aug 2015 16:34:03 +0300 Subject: [PATCH 05/49] Fix test warnings --- plottable.js | 4 +--- src/plots/barPlot.ts | 6 ++---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/plottable.js b/plottable.js index 58d68a99de..a7052f6fe1 100644 --- a/plottable.js +++ b/plottable.js @@ -7911,8 +7911,6 @@ var Plottable; var y = attrToProjector["y"](d, i, dataset); var positive = originalPositionFn(d, i, dataset) <= scaledBaseline; var measurement = measurer.measure(text); - var color = attrToProjector["fill"](d, i, dataset); - var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); var primary = _this._isVertical ? h : w; var primarySpace = _this._isVertical ? measurement.height : measurement.width; var secondaryAttrTextSpace = _this._isVertical ? measurement.width : measurement.height; @@ -8118,7 +8116,7 @@ var Plottable; Plottable.Utils.Math.isValidNumber(attrToProjector["y"](d, i, dataset)) && Plottable.Utils.Math.isValidNumber(attrToProjector["width"](d, i, dataset)) && Plottable.Utils.Math.isValidNumber(attrToProjector["height"](d, i, dataset)) && - (primaryAccessor(d, i, dataset) != 0); }); + (primaryAccessor(d, i, dataset) !== 0); }); dataToDraw.set(dataset, data); }); return dataToDraw; diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index fd019b0724..552492569f 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -424,8 +424,6 @@ export module Plots { let y = attrToProjector["y"](d, i, dataset); let positive = originalPositionFn(d, i, dataset) <= scaledBaseline; let measurement = measurer.measure(text); - let color = attrToProjector["fill"](d, i, dataset); - let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); let primary = this._isVertical ? h : w; let primarySpace = this._isVertical ? measurement.height : measurement.width; let secondaryAttrTextSpace = this._isVertical ? measurement.width : measurement.height; @@ -446,7 +444,7 @@ export module Plots { y: y }; - if(showLabelAboveBar) { + if (showLabelAboveBar) { y = y - h + offset; labelPosition.y = y; } else { @@ -640,7 +638,7 @@ export module Plots { Utils.Math.isValidNumber(attrToProjector["y"](d, i, dataset)) && Utils.Math.isValidNumber(attrToProjector["width"](d, i, dataset)) && Utils.Math.isValidNumber(attrToProjector["height"](d, i, dataset)) && - (primaryAccessor(d, i, dataset) != 0)); + (primaryAccessor(d, i, dataset) !== 0)); dataToDraw.set(dataset, data); }); return dataToDraw; From 56c1c801b1551cd1bc1fd796e0c758f03c7334b8 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 12 Aug 2015 12:16:29 +0300 Subject: [PATCH 06/49] Clean up calculation of y --- plottable.js | 34 ++++++++++++++++------------------ src/plots/barPlot.ts | 33 +++++++++++++++++---------------- 2 files changed, 33 insertions(+), 34 deletions(-) diff --git a/plottable.js b/plottable.js index a7052f6fe1..52fc6b33e0 100644 --- a/plottable.js +++ b/plottable.js @@ -7908,7 +7908,7 @@ var Plottable; var w = attrToProjector["width"](d, i, dataset); var h = attrToProjector["height"](d, i, dataset); var x = attrToProjector["x"](d, i, dataset); - var y = attrToProjector["y"](d, i, dataset); + var baselineY = attrToProjector["y"](d, i, dataset); var positive = originalPositionFn(d, i, dataset) <= scaledBaseline; var measurement = measurer.measure(text); var primary = _this._isVertical ? h : w; @@ -7916,30 +7916,28 @@ var Plottable; var secondaryAttrTextSpace = _this._isVertical ? measurement.width : measurement.height; var secondaryAttrAvailableSpace = _this._isVertical ? w : h; var tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - var showLabelAboveBar = (measurement.height > h); + var showLabelOffBar = (measurement.height > h); if (measurement.width <= w) { var offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { offset = offset * -1; } - if (_this._isVertical) { - y += offset; - } - else { - x += offset; - } + var yCalculator = function () { + var addend = offset; + if (showLabelOffBar && positive) { + addend += (offset - h); + } + if (showLabelOffBar && !positive) { + addend += measurement.height; + } + ; + return baselineY + addend; + }; + var y = yCalculator(); var labelPosition = { x: x, - y: y + y: positive ? y : y + h - measurement.height }; - if (showLabelAboveBar) { - y = y - h + offset; - labelPosition.y = y; - } - else { - labelPosition.y = positive ? y : y + h - measurement.height; - } - ; if (_this._isVertical) { labelPosition.x = x + w / 2 - measurement.width / 2; } @@ -7952,7 +7950,7 @@ var Plottable; } } var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - var labelPositioningClassName = showLabelAboveBar ? "above-bar-label" : "in-bar-label"; + var labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; g.classed(labelPositioningClassName, true); g.style("visibility", "inherit"); var xAlign; diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 552492569f..f41b6cb496 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -421,7 +421,7 @@ export module Plots { let w = attrToProjector["width"](d, i, dataset); let h = attrToProjector["height"](d, i, dataset); let x = attrToProjector["x"](d, i, dataset); - let y = attrToProjector["y"](d, i, dataset); + let baselineY = attrToProjector["y"](d, i, dataset); let positive = originalPositionFn(d, i, dataset) <= scaledBaseline; let measurement = measurer.measure(text); let primary = this._isVertical ? h : w; @@ -429,26 +429,27 @@ export module Plots { let secondaryAttrTextSpace = this._isVertical ? measurement.width : measurement.height; let secondaryAttrAvailableSpace = this._isVertical ? w : h; let tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - let showLabelAboveBar = (measurement.height > h); + let showLabelOffBar = (measurement.height > h); if (measurement.width <= w) { let offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { offset = offset * -1; } - if (this._isVertical) { - y += offset; - } else { - x += offset; - } - let labelPosition = { - x: x, - y: y + let yCalculator = () => { + let addend = offset; + if (showLabelOffBar && positive) { + addend += (offset - h); + } + if (showLabelOffBar && !positive) { + addend += measurement.height + }; + return baselineY + addend; }; - if (showLabelAboveBar) { - y = y - h + offset; - labelPosition.y = y; - } else { - labelPosition.y = positive ? y : y + h - measurement.height; + let y = yCalculator(); + + let labelPosition = { + x: x, + y: positive ? y : y + h - measurement.height }; if (this._isVertical) { @@ -462,7 +463,7 @@ export module Plots { } let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - let labelPositioningClassName = showLabelAboveBar ? "above-bar-label" : "in-bar-label"; + let labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; g.classed(labelPositioningClassName, true); g.style("visibility", "inherit"); From 7e2c26acc17dcf04bd9b9ea47393a04fe05be5b6 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 12 Aug 2015 12:16:54 +0300 Subject: [PATCH 07/49] Use more appropriate CSS class names --- plottable.css | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/plottable.css b/plottable.css index 9ccc71364a..91eb1d0ead 100644 --- a/plottable.css +++ b/plottable.css @@ -65,16 +65,16 @@ svg.plottable { } .plottable .label-area text { - fill: #32313F; + fill: white; font-family: "Helvetica Neue", sans-serif; font-size: 14px; } -.plottable .above-bar-label text { +.plottable .off-bar-label text { fill: #5279c7; } -.plottable .in-bar-label text { +.plottable .on-bar-label text { fill: white; } From 63facda9cf87d68fbc13d2d910913a0923b54429 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 12 Aug 2015 12:29:41 +0300 Subject: [PATCH 08/49] Remove unused labelPosition binding --- plottable.js | 15 --------------- src/plots/barPlot.ts | 15 --------------- 2 files changed, 30 deletions(-) diff --git a/plottable.js b/plottable.js index 52fc6b33e0..6d269c14e5 100644 --- a/plottable.js +++ b/plottable.js @@ -7934,21 +7934,6 @@ var Plottable; return baselineY + addend; }; var y = yCalculator(); - var labelPosition = { - x: x, - y: positive ? y : y + h - measurement.height - }; - if (_this._isVertical) { - labelPosition.x = x + w / 2 - measurement.width / 2; - } - else { - if (!positive) { - labelPosition.x = x + w - measurement.width; - } - else { - labelPosition.x = x; - } - } var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); var labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; g.classed(labelPositioningClassName, true); diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index f41b6cb496..e2ff2a5d55 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -447,21 +447,6 @@ export module Plots { let y = yCalculator(); - let labelPosition = { - x: x, - y: positive ? y : y + h - measurement.height - }; - - if (this._isVertical) { - labelPosition.x = x + w / 2 - measurement.width / 2; - } else { - if (!positive) { - labelPosition.x = x + w - measurement.width; - } else { - labelPosition.x = x; - } - } - let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); let labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; g.classed(labelPositioningClassName, true); From 3d55666cdc47dbb599eb6372deb6e8a6856c52a7 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 12 Aug 2015 12:48:38 +0300 Subject: [PATCH 09/49] Add support for off bar labels on horizontal bar plots --- plottable.js | 42 +++++++++++++++++++++++++++++------------- src/plots/barPlot.ts | 39 +++++++++++++++++++++++++++------------ 2 files changed, 56 insertions(+), 25 deletions(-) diff --git a/plottable.js b/plottable.js index 6d269c14e5..4d213b1937 100644 --- a/plottable.js +++ b/plottable.js @@ -7907,7 +7907,7 @@ var Plottable; var text = _this._labelFormatter(primaryAccessor(d, i, dataset)).toString(); var w = attrToProjector["width"](d, i, dataset); var h = attrToProjector["height"](d, i, dataset); - var x = attrToProjector["x"](d, i, dataset); + var baselineX = attrToProjector["x"](d, i, dataset); var baselineY = attrToProjector["y"](d, i, dataset); var positive = originalPositionFn(d, i, dataset) <= scaledBaseline; var measurement = measurer.measure(text); @@ -7916,25 +7916,41 @@ var Plottable; var secondaryAttrTextSpace = _this._isVertical ? measurement.width : measurement.height; var secondaryAttrAvailableSpace = _this._isVertical ? w : h; var tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - var showLabelOffBar = (measurement.height > h); - if (measurement.width <= w) { + var showLabelOffBar = _this._isVertical ? (measurement.height > h) : (measurement.width > w); + if (true) { var offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { offset = offset * -1; } - var yCalculator = function () { - var addend = offset; - if (showLabelOffBar && positive) { - addend += (offset - h); - } - if (showLabelOffBar && !positive) { - addend += measurement.height; + var getY = function () { + var addend = 0; + if (_this._isVertical) { + addend += offset; + if (showLabelOffBar && positive) { + addend += (offset - h); + } + if (showLabelOffBar && !positive) { + addend += measurement.height; + } + ; } - ; return baselineY + addend; }; - var y = yCalculator(); - var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); + var getX = function () { + var addend = 0; + if (!_this._isVertical) { + addend += offset; + if (showLabelOffBar && positive) { + addend += (offset - w); + } + if (showLabelOffBar && !positive) { + addend += measurement.width; + } + ; + } + return baselineX + addend; + }; + var g = labelArea.append("g").attr("transform", "translate(" + getX() + "," + getY() + ")"); var labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; g.classed(labelPositioningClassName, true); g.style("visibility", "inherit"); diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index e2ff2a5d55..7af68e18a2 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -420,7 +420,7 @@ export module Plots { let text = this._labelFormatter(primaryAccessor(d, i, dataset)).toString(); let w = attrToProjector["width"](d, i, dataset); let h = attrToProjector["height"](d, i, dataset); - let x = attrToProjector["x"](d, i, dataset); + let baselineX = attrToProjector["x"](d, i, dataset); let baselineY = attrToProjector["y"](d, i, dataset); let positive = originalPositionFn(d, i, dataset) <= scaledBaseline; let measurement = measurer.measure(text); @@ -429,25 +429,40 @@ export module Plots { let secondaryAttrTextSpace = this._isVertical ? measurement.width : measurement.height; let secondaryAttrAvailableSpace = this._isVertical ? w : h; let tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - let showLabelOffBar = (measurement.height > h); - if (measurement.width <= w) { + let showLabelOffBar = this._isVertical ? (measurement.height > h) : (measurement.width > w) + if (true) { let offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { offset = offset * -1; } - let yCalculator = () => { - let addend = offset; - if (showLabelOffBar && positive) { - addend += (offset - h); + let getY = () => { + let addend = 0; + if (this._isVertical) { + addend += offset; + if (showLabelOffBar && positive) { + addend += (offset - h); + } + if (showLabelOffBar && !positive) { + addend += measurement.height + }; } - if (showLabelOffBar && !positive) { - addend += measurement.height - }; return baselineY + addend; }; - let y = yCalculator(); + let getX = () => { + let addend = 0; + if (!this._isVertical) { + addend += offset; + if (showLabelOffBar && positive) { + addend += (offset - w); + } + if (showLabelOffBar && !positive) { + addend += measurement.width + }; + } + return baselineX + addend; + }; - let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); + let g = labelArea.append("g").attr("transform", "translate(" + getX() + "," + getY() + ")"); let labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; g.classed(labelPositioningClassName, true); From e6bdc3477f88e488307466b565ec3b31b61bd081 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 12 Aug 2015 13:11:45 +0300 Subject: [PATCH 10/49] Increase gap between negative off-bar label and bar --- plottable.js | 2 +- src/plots/barPlot.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/plottable.js b/plottable.js index 4d213b1937..06eeddc5ba 100644 --- a/plottable.js +++ b/plottable.js @@ -7941,7 +7941,7 @@ var Plottable; if (!_this._isVertical) { addend += offset; if (showLabelOffBar && positive) { - addend += (offset - w); + addend += (offset - w - Bar._LABEL_HORIZONTAL_PADDING); } if (showLabelOffBar && !positive) { addend += measurement.width; diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 7af68e18a2..5859a64ca1 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -453,10 +453,10 @@ export module Plots { if (!this._isVertical) { addend += offset; if (showLabelOffBar && positive) { - addend += (offset - w); + addend += (offset - w - Bar._LABEL_HORIZONTAL_PADDING); } if (showLabelOffBar && !positive) { - addend += measurement.width + addend += measurement.width; }; } return baselineX + addend; From 1925d39b7bc92da135402a8d6eb607d04d320414 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 12 Aug 2015 16:50:35 +0300 Subject: [PATCH 11/49] All labels are visible by default --- test/plots/barPlotTests.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 6f25e32b2f..fd3c1d655f 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -696,9 +696,9 @@ describe("Plots", () => { plot.labelsEnabled(true); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); assert.lengthOf(texts, 3, "all texts drawn"); - assert.strictEqual(texts[0], "640", "first label is 640"); - assert.strictEqual(texts[1], "12345", "first label is 12345"); - assert.strictEqual(texts[2], "5", "first label is 5"); + assert.strictEqual(texts[0], "5", "first label is 640"); + assert.strictEqual(texts[1], "640", "first label is 640"); + assert.strictEqual(texts[2], "12345", "first label is 12345"); svg.remove(); }); From e8899d4c8eb33750da44599741c121025d8d2265 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 12 Aug 2015 17:08:10 +0300 Subject: [PATCH 12/49] Set correct expected label count --- test/plots/barPlotTests.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index fd3c1d655f..645844806c 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -734,7 +734,7 @@ describe("Plots", () => { if (!called) { originalDrawLabels.apply(plot); texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 2, "texts were repopulated by drawLabels after the update"); + assert.lengthOf(texts, 3, "texts were repopulated by drawLabels after the update"); svg.remove(); called = true; // for some reason, in phantomJS, `done` was being called multiple times and this caused the test to fail. done(); From 1e7fd9018cf302ce9574067b294cfa5ba142a316 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 12 Aug 2015 17:15:34 +0300 Subject: [PATCH 13/49] Remove test Likely unnecessary, as labels outside visible render area will not be seen to begin with. --- test/plots/barPlotTests.ts | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 645844806c..cebd4512af 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -769,23 +769,6 @@ describe("Plots", () => { plot.renderTo(svg); }); - it("hides labels outside of the visible render area (horizontal)", () => { - xScale.domain([1, 3]); - - let texts = svg.selectAll("text"); - assert.strictEqual(texts.size(), plot.datasets()[0].data().length, "One label rendered for each piece of data"); - - let label1 = d3.select(texts[0][0]); - let label2 = d3.select(texts[0][1]); - let label3 = d3.select(texts[0][2]); - - assert.strictEqual(label1.style("visibility"), "hidden", "Left label is cut off by the margin"); - assert.include(["visible", "inherit"], label2.style("visibility"), "Middle label should still show"); - assert.strictEqual(label3.style("visibility"), "hidden", "Right label is cut off by the margin"); - - svg.remove(); - }); - it("hides labels outside of the visible render area (vertical)", () => { yScale.domain([2.5, 11]); From 86fa47ae9a87a7ce44398bba582e28c2d9e6ee0b Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 12 Aug 2015 17:40:55 +0300 Subject: [PATCH 14/49] [WIP] Restore modified version of label hiding code --- plottable.js | 7 +++++-- src/plots/barPlot.ts | 10 ++++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/plottable.js b/plottable.js index 06eeddc5ba..4024a7846f 100644 --- a/plottable.js +++ b/plottable.js @@ -7950,10 +7950,13 @@ var Plottable; } return baselineX + addend; }; - var g = labelArea.append("g").attr("transform", "translate(" + getX() + "," + getY() + ")"); + var x = getX(); + var y = getY(); + var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); var labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; g.classed(labelPositioningClassName, true); - g.style("visibility", "inherit"); + var hideLabel = (x + measurement.width > _this.width() || (positive ? y + measurement.height : y + h) > _this.height()); + g.style("visibility", hideLabel ? "hidden" : "inherit"); var xAlign; var yAlign; if (_this._isVertical) { diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 5859a64ca1..25d3bc493a 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -462,11 +462,17 @@ export module Plots { return baselineX + addend; }; - let g = labelArea.append("g").attr("transform", "translate(" + getX() + "," + getY() + ")"); + let x = getX(); + let y = getY(); + + let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); let labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; + g.classed(labelPositioningClassName, true); - g.style("visibility", "inherit"); + let hideLabel = (x + measurement.width > this.width() || (positive ? y + measurement.height : y + h) > this.height()); + + g.style("visibility", hideLabel ? "hidden" : "inherit"); let xAlign: string; let yAlign: string; if (this._isVertical) { From 51fcf7be2f135e23f65fcd6ab2c062fa2011646c Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Mon, 17 Aug 2015 14:10:30 +0300 Subject: [PATCH 15/49] Remove zero-filter --- plottable.js | 3 +-- src/plots/barPlot.ts | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/plottable.js b/plottable.js index 4024a7846f..fd147ba616 100644 --- a/plottable.js +++ b/plottable.js @@ -8117,8 +8117,7 @@ var Plottable; var data = dataset.data().filter(function (d, i) { return Plottable.Utils.Math.isValidNumber(attrToProjector["x"](d, i, dataset)) && Plottable.Utils.Math.isValidNumber(attrToProjector["y"](d, i, dataset)) && Plottable.Utils.Math.isValidNumber(attrToProjector["width"](d, i, dataset)) && - Plottable.Utils.Math.isValidNumber(attrToProjector["height"](d, i, dataset)) && - (primaryAccessor(d, i, dataset) !== 0); }); + Plottable.Utils.Math.isValidNumber(attrToProjector["height"](d, i, dataset)); }); dataToDraw.set(dataset, data); }); return dataToDraw; diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 25d3bc493a..2117d446ba 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -644,8 +644,7 @@ export module Plots { let data = dataset.data().filter((d, i) => Utils.Math.isValidNumber(attrToProjector["x"](d, i, dataset)) && Utils.Math.isValidNumber(attrToProjector["y"](d, i, dataset)) && Utils.Math.isValidNumber(attrToProjector["width"](d, i, dataset)) && - Utils.Math.isValidNumber(attrToProjector["height"](d, i, dataset)) && - (primaryAccessor(d, i, dataset) !== 0)); + Utils.Math.isValidNumber(attrToProjector["height"](d, i, dataset))); dataToDraw.set(dataset, data); }); return dataToDraw; From b66d81620fb61df1a0cd3ac5424f7517499a4822 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Mon, 17 Aug 2015 14:22:31 +0300 Subject: [PATCH 16/49] Restore dark-label/light-label classes --- plottable.css | 8 ++++++++ plottable.js | 3 +++ src/plots/barPlot.ts | 5 +++++ 3 files changed, 16 insertions(+) diff --git a/plottable.css b/plottable.css index 91eb1d0ead..5931948651 100644 --- a/plottable.css +++ b/plottable.css @@ -70,6 +70,14 @@ svg.plottable { font-size: 14px; } +.plottable .light-label text { + fill: white; +} + +.plottable .dark-label text { + fill: #32313F; +} + .plottable .off-bar-label text { fill: #5279c7; } diff --git a/plottable.js b/plottable.js index fd147ba616..310f9156e1 100644 --- a/plottable.js +++ b/plottable.js @@ -7954,6 +7954,9 @@ var Plottable; var y = getY(); var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); var labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; + var color = attrToProjector["fill"](d, i, dataset); + var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); + g.classed(dark ? "dark-label" : "light-label"); g.classed(labelPositioningClassName, true); var hideLabel = (x + measurement.width > _this.width() || (positive ? y + measurement.height : y + h) > _this.height()); g.style("visibility", hideLabel ? "hidden" : "inherit"); diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 2117d446ba..23c42014c3 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -468,6 +468,11 @@ export module Plots { let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); let labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; + let color = attrToProjector["fill"](d, i, dataset); + let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); + + g.classed(dark ? "dark-label" : "light-label"); + g.classed(labelPositioningClassName, true); let hideLabel = (x + measurement.width > this.width() || (positive ? y + measurement.height : y + h) > this.height()); From 866b8514df8f603676c46f68aab227a0c72c4f08 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Mon, 17 Aug 2015 14:45:53 +0300 Subject: [PATCH 17/49] Substitute previous tests with more relevant one --- test/plots/barPlotTests.ts | 31 +++++++------------------------ 1 file changed, 7 insertions(+), 24 deletions(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index cebd4512af..674b7702dd 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -611,7 +611,7 @@ describe("Plots", () => { }); }); - describe("Horizontal Bar Plot label visibility", () => { + describe("Horizontal Bar Plot With Bar Labels", () => { let svg: d3.Selection; let yScale: Plottable.Scales.Category; let xScale: Plottable.Scales.Linear; @@ -623,7 +623,7 @@ describe("Plots", () => { let data = [ {y: "A", x: -1.5}, - {y: "B", x: 1}, + {y: "B", x: 100}, ]; barPlot = new Plottable.Plots.Bar(Plottable.Plots.Bar.ORIENTATION_HORIZONTAL); @@ -634,32 +634,15 @@ describe("Plots", () => { barPlot.renderTo(svg); }); - it("hides labels properly on the right", () => { - xScale.domainMax(0.95); + it("shows both inner and outer labels", () => { let texts = svg.selectAll("text"); - - assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); - - let label1 = d3.select(texts[0][0]); - let label2 = d3.select(texts[0][1]); - - assert.include(["visible", "inherit"], label1.style("visibility"), "label 1 is visible"); - assert.strictEqual(label2.style("visibility"), "hidden", "label 2 is not visible"); - - svg.remove(); - }); - - it("hides labels properly on the left", () => { - xScale.domainMin(-1.4); - let texts = svg.selectAll("text"); - assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); - let label1 = d3.select(texts[0][0]); - let label2 = d3.select(texts[0][1]); + let offBarLabelCount = d3.selectAll(".off-bar-label")[0].length; + assert.strictEqual(offBarLabelCount, 1, "There should be 1 labels rendered outside the bar"); - assert.strictEqual(label1.style("visibility"), "hidden", "label 2 is not visible"); - assert.include(["visible", "inherit"], label2.style("visibility"), "label 1 is visible"); + let onBarLabelCount = d3.selectAll(".on-bar-label")[0].length; + assert.strictEqual(onBarLabelCount, 1, "There should be 1 labels rendered inside the bar"); svg.remove(); }); }); From 4895bad13cdafcdee40ff0d41ba30c34d1bdc27d Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Mon, 17 Aug 2015 15:10:24 +0300 Subject: [PATCH 18/49] Add test to confirm that labels are shown as appropriate for vertical bars --- test/plots/barPlotTests.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 674b7702dd..5c194677aa 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -706,6 +706,20 @@ describe("Plots", () => { svg.remove(); }); + it("bar labels are shown inside or outside the bar as appropriate", () => { + plot.labelsEnabled(true); + plot.renderTo(svg); + plot.renderTo(svg); + let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); + + let offBarLabelCount = d3.selectAll(".off-bar-label")[0].length; + assert.strictEqual(offBarLabelCount, 1, "There should be 1 label rendered outside the bar"); + + let onBarLabelCount = d3.selectAll(".on-bar-label")[0].length; + assert.strictEqual(onBarLabelCount, 2, "There should be 2 labels rendered inside the bar"); + svg.remove(); + }); + it("bar labels are removed instantly on dataset change", (done) => { plot.labelsEnabled(true); plot.renderTo(svg); From 3b8930d76cc3c34edfed6f40c43c43c7551a2b32 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Mon, 17 Aug 2015 15:42:44 +0300 Subject: [PATCH 19/49] Restore previous API behaviour with additional flag displayLabelsOffBar --- plottable.d.ts | 13 +++++++++++++ plottable.js | 13 ++++++++++++- src/plots/barPlot.ts | 29 +++++++++++++++++++++++++++-- test/plots/barPlotTests.ts | 25 +++++++++++++------------ 4 files changed, 65 insertions(+), 15 deletions(-) diff --git a/plottable.d.ts b/plottable.d.ts index 8a57aee376..499ca982b6 100644 --- a/plottable.d.ts +++ b/plottable.d.ts @@ -3017,6 +3017,19 @@ declare module Plottable { * @returns {Bar} The calling Bar Plot. */ labelsEnabled(enabled: boolean): Bar; + /** + * Get whether to display labels off the bar + * + * @returns {boolean} Whether labels should be displayed off the bar + */ + displayLabelsOffBar(): boolean; + /** + * Sets whether labels are displayed off the bar + * + * @param {boolean} displayLabelsOffBar + * @return {Bar} The calling Bar Plot. + */ + displayLabelsOffBar(display: boolean): Bar; /** * Gets the Formatter for the labels. */ diff --git a/plottable.js b/plottable.js index 310f9156e1..d8f3f3cf0c 100644 --- a/plottable.js +++ b/plottable.js @@ -7615,6 +7615,7 @@ var Plottable; _super.call(this); this._labelFormatter = Plottable.Formatters.identity(); this._labelsEnabled = false; + this._displayLabelsOffBar = false; this._hideBarsIfAnyAreTooWide = true; this._barPixelWidth = 0; this.addClass("bar-plot"); @@ -7721,6 +7722,16 @@ var Plottable; return this; } }; + Bar.prototype.displayLabelsOffBar = function (display) { + if (display === undefined) { + return this._displayLabelsOffBar; + } + else { + this._displayLabelsOffBar = display; + this.render(); + return this; + } + }; Bar.prototype.labelFormatter = function (formatter) { if (formatter == null) { return this._labelFormatter; @@ -7917,7 +7928,7 @@ var Plottable; var secondaryAttrAvailableSpace = _this._isVertical ? w : h; var tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; var showLabelOffBar = _this._isVertical ? (measurement.height > h) : (measurement.width > w); - if (true) { + if ((measurement.height <= h && measurement.width <= w) || _this._displayLabelsOffBar) { var offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { offset = offset * -1; diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 23c42014c3..b517e59f94 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -23,6 +23,7 @@ export module Plots { protected _isVertical: boolean; private _labelFormatter: Formatter = Formatters.identity(); private _labelsEnabled = false; + private _displayLabelsOffBar = false; private _hideBarsIfAnyAreTooWide = true; private _labelConfig: Utils.Map; private _baselineValueProvider: () => (X|Y)[]; @@ -190,6 +191,30 @@ export module Plots { } } + /** + * Get whether to display labels off the bar + * + * @returns {boolean} Whether labels should be displayed off the bar + */ + public displayLabelsOffBar(): boolean; + + /** + * Sets whether labels are displayed off the bar + * + * @param {boolean} displayLabelsOffBar + * @return {Bar} The calling Bar Plot. + */ + public displayLabelsOffBar(display: boolean): Bar ; + public displayLabelsOffBar(display?: boolean): any { + if (display === undefined) { + return this._displayLabelsOffBar; + } else { + this._displayLabelsOffBar = display; + this.render(); + return this; + } + } + /** * Gets the Formatter for the labels. */ @@ -429,8 +454,8 @@ export module Plots { let secondaryAttrTextSpace = this._isVertical ? measurement.width : measurement.height; let secondaryAttrAvailableSpace = this._isVertical ? w : h; let tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - let showLabelOffBar = this._isVertical ? (measurement.height > h) : (measurement.width > w) - if (true) { + let showLabelOffBar = this._isVertical ? (measurement.height > h) : (measurement.width > w); + if ((measurement.height <= h && measurement.width <= w) || this._displayLabelsOffBar) { let offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { offset = offset * -1; } diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 5c194677aa..dc42956f4e 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -635,6 +635,9 @@ describe("Plots", () => { }); it("shows both inner and outer labels", () => { + barPlot.displayLabelsOffBar(true); + barPlot.renderTo(svg); + let texts = svg.selectAll("text"); assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); @@ -678,10 +681,9 @@ describe("Plots", () => { plot.renderTo(svg); plot.labelsEnabled(true); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 3, "all texts drawn"); - assert.strictEqual(texts[0], "5", "first label is 640"); - assert.strictEqual(texts[1], "640", "first label is 640"); - assert.strictEqual(texts[2], "12345", "first label is 12345"); + assert.lengthOf(texts, 2, "both texts drawn"); + assert.strictEqual(texts[0], "640", "first label is 640"); + assert.strictEqual(texts[1], "12345", "first label is 12345"); svg.remove(); }); @@ -699,17 +701,16 @@ describe("Plots", () => { plot.labelFormatter((n: number) => n.toString() + "%"); plot.renderTo(svg); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 3, "all texts drawn"); - assert.strictEqual(texts[0], "5%", "first label is 5%"); - assert.strictEqual(texts[1], "640%", "first label is 640%"); - assert.strictEqual(texts[2], "12345%", "first label is 12345%"); + assert.lengthOf(texts, 2, "both texts drawn"); + assert.strictEqual(texts[0], "640%", "first label is 640%"); + assert.strictEqual(texts[1], "12345%", "first label is 12345%"); svg.remove(); }); it("bar labels are shown inside or outside the bar as appropriate", () => { + plot.displayLabelsOffBar(true); plot.labelsEnabled(true); plot.renderTo(svg); - plot.renderTo(svg); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); let offBarLabelCount = d3.selectAll(".off-bar-label")[0].length; @@ -724,14 +725,14 @@ describe("Plots", () => { plot.labelsEnabled(true); plot.renderTo(svg); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 3, "all texts drawn"); + assert.lengthOf(texts, 2, "both texts drawn"); let originalDrawLabels = ( plot)._drawLabels; let called = false; ( plot)._drawLabels = () => { if (!called) { originalDrawLabels.apply(plot); texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 3, "texts were repopulated by drawLabels after the update"); + assert.lengthOf(texts, 2, "texts were repopulated by drawLabels after the update"); svg.remove(); called = true; // for some reason, in phantomJS, `done` was being called multiple times and this caused the test to fail. done(); @@ -739,7 +740,7 @@ describe("Plots", () => { }; dataset.data(data); texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 0, "texts were immediately removed"); + assert.lengthOf(texts, 2, "texts were immediately removed"); }); }); From 8bec0c0180ef85fc2241913f95ddf49d8af0f713 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Mon, 17 Aug 2015 15:45:00 +0300 Subject: [PATCH 20/49] Restore previous default label text colour --- plottable.css | 2 +- plottable.js | 14 +++++++++----- src/plots/barPlot.ts | 13 ++++++++----- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/plottable.css b/plottable.css index 5931948651..8260f8a2bc 100644 --- a/plottable.css +++ b/plottable.css @@ -65,7 +65,7 @@ svg.plottable { } .plottable .label-area text { - fill: white; + fill: #32313F; font-family: "Helvetica Neue", sans-serif; font-size: 14px; } diff --git a/plottable.js b/plottable.js index d8f3f3cf0c..71197ee9f6 100644 --- a/plottable.js +++ b/plottable.js @@ -7964,11 +7964,15 @@ var Plottable; var x = getX(); var y = getY(); var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - var labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; - var color = attrToProjector["fill"](d, i, dataset); - var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); - g.classed(dark ? "dark-label" : "light-label"); - g.classed(labelPositioningClassName, true); + if (_this._displayLabelsOffBar) { + var labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; + g.classed(labelPositioningClassName, true); + } + else { + var color = attrToProjector["fill"](d, i, dataset); + var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); + g.classed(dark ? "dark-label" : "light-label"); + } var hideLabel = (x + measurement.width > _this.width() || (positive ? y + measurement.height : y + h) > _this.height()); g.style("visibility", hideLabel ? "hidden" : "inherit"); var xAlign; diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index b517e59f94..1adad576cd 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -491,14 +491,17 @@ export module Plots { let y = getY(); let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - let labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; - let color = attrToProjector["fill"](d, i, dataset); - let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); + if (this._displayLabelsOffBar) { + let labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; + g.classed(labelPositioningClassName, true); + } else { + let color = attrToProjector["fill"](d, i, dataset); + let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); + g.classed(dark ? "dark-label" : "light-label"); + } - g.classed(dark ? "dark-label" : "light-label"); - g.classed(labelPositioningClassName, true); let hideLabel = (x + measurement.width > this.width() || (positive ? y + measurement.height : y + h) > this.height()); From e3a805362a309ab49c259defa7c2f10bff1fefbc Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Mon, 17 Aug 2015 15:52:36 +0300 Subject: [PATCH 21/49] Fix broken test --- test/plots/barPlotTests.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index dc42956f4e..134b70a84b 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -740,7 +740,7 @@ describe("Plots", () => { }; dataset.data(data); texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 2, "texts were immediately removed"); + assert.lengthOf(texts, 0, "texts were immediately removed"); }); }); From 35b52ce202dbb463af357022d53b9d5a011ec5ba Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Mon, 17 Aug 2015 16:01:55 +0300 Subject: [PATCH 22/49] Add missing boolean parameter --- plottable.js | 2 +- src/plots/barPlot.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plottable.js b/plottable.js index 71197ee9f6..5441daae90 100644 --- a/plottable.js +++ b/plottable.js @@ -7971,7 +7971,7 @@ var Plottable; else { var color = attrToProjector["fill"](d, i, dataset); var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); - g.classed(dark ? "dark-label" : "light-label"); + g.classed(dark ? "dark-label" : "light-label", true); } var hideLabel = (x + measurement.width > _this.width() || (positive ? y + measurement.height : y + h) > _this.height()); g.style("visibility", hideLabel ? "hidden" : "inherit"); diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 1adad576cd..fca7eb3bb3 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -498,7 +498,7 @@ export module Plots { } else { let color = attrToProjector["fill"](d, i, dataset); let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); - g.classed(dark ? "dark-label" : "light-label"); + g.classed(dark ? "dark-label" : "light-label", true); } From 71d0fc6b07d60c9bff8725d562aaf68c4d9f3529 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Mon, 17 Aug 2015 16:10:56 +0300 Subject: [PATCH 23/49] Fix tslint violations --- plottable.js | 1 - src/plots/barPlot.ts | 5 +---- test/plots/barPlotTests.ts | 2 ++ 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/plottable.js b/plottable.js index 5441daae90..9b6ea55297 100644 --- a/plottable.js +++ b/plottable.js @@ -8130,7 +8130,6 @@ var Plottable; Bar.prototype._getDataToDraw = function () { var dataToDraw = new Plottable.Utils.Map(); var attrToProjector = this._generateAttrToProjector(); - var primaryAccessor = this._isVertical ? this.y().accessor : this.x().accessor; this.datasets().forEach(function (dataset) { var data = dataset.data().filter(function (d, i) { return Plottable.Utils.Math.isValidNumber(attrToProjector["x"](d, i, dataset)) && Plottable.Utils.Math.isValidNumber(attrToProjector["y"](d, i, dataset)) && diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index fca7eb3bb3..59aaa2c5dc 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -467,7 +467,7 @@ export module Plots { addend += (offset - h); } if (showLabelOffBar && !positive) { - addend += measurement.height + addend += measurement.height; }; } return baselineY + addend; @@ -501,8 +501,6 @@ export module Plots { g.classed(dark ? "dark-label" : "light-label", true); } - - let hideLabel = (x + measurement.width > this.width() || (positive ? y + measurement.height : y + h) > this.height()); g.style("visibility", hideLabel ? "hidden" : "inherit"); @@ -672,7 +670,6 @@ export module Plots { protected _getDataToDraw() { let dataToDraw = new Utils.Map(); let attrToProjector = this._generateAttrToProjector(); - let primaryAccessor = this._isVertical ? this.y().accessor : this.x().accessor; this.datasets().forEach((dataset: Dataset) => { let data = dataset.data().filter((d, i) => Utils.Math.isValidNumber(attrToProjector["x"](d, i, dataset)) && Utils.Math.isValidNumber(attrToProjector["y"](d, i, dataset)) && diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 134b70a84b..b88fdc83d6 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -711,7 +711,9 @@ describe("Plots", () => { plot.displayLabelsOffBar(true); plot.labelsEnabled(true); plot.renderTo(svg); + let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); + assert.lengthOf(texts, 3, "both texts drawn"); let offBarLabelCount = d3.selectAll(".off-bar-label")[0].length; assert.strictEqual(offBarLabelCount, 1, "There should be 1 label rendered outside the bar"); From 2170fd929ae2b9b29eb7421f4e976f13a1390809 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 19 Aug 2015 10:38:11 +0300 Subject: [PATCH 24/49] Remove styling for .on-bar-label, change color of .off-bar-label --- plottable.css | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/plottable.css b/plottable.css index 8260f8a2bc..f015cf2a05 100644 --- a/plottable.css +++ b/plottable.css @@ -79,11 +79,7 @@ svg.plottable { } .plottable .off-bar-label text { - fill: #5279c7; -} - -.plottable .on-bar-label text { - fill: white; + fill: #32313F; } .plottable .axis-label text { From ed486352edcbb0ed367a38b6f70fdf184139d46f Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 19 Aug 2015 11:07:16 +0300 Subject: [PATCH 25/49] Remove displayLabelsOffBar flag --- plottable.d.ts | 13 ---- plottable.js | 125 ++++++++++++++----------------- src/plots/barPlot.ts | 147 +++++++++++++++---------------------- test/plots/barPlotTests.ts | 20 ++--- 4 files changed, 123 insertions(+), 182 deletions(-) diff --git a/plottable.d.ts b/plottable.d.ts index 499ca982b6..8a57aee376 100644 --- a/plottable.d.ts +++ b/plottable.d.ts @@ -3017,19 +3017,6 @@ declare module Plottable { * @returns {Bar} The calling Bar Plot. */ labelsEnabled(enabled: boolean): Bar; - /** - * Get whether to display labels off the bar - * - * @returns {boolean} Whether labels should be displayed off the bar - */ - displayLabelsOffBar(): boolean; - /** - * Sets whether labels are displayed off the bar - * - * @param {boolean} displayLabelsOffBar - * @return {Bar} The calling Bar Plot. - */ - displayLabelsOffBar(display: boolean): Bar; /** * Gets the Formatter for the labels. */ diff --git a/plottable.js b/plottable.js index 9b6ea55297..0fdb4e5c7d 100644 --- a/plottable.js +++ b/plottable.js @@ -7615,7 +7615,6 @@ var Plottable; _super.call(this); this._labelFormatter = Plottable.Formatters.identity(); this._labelsEnabled = false; - this._displayLabelsOffBar = false; this._hideBarsIfAnyAreTooWide = true; this._barPixelWidth = 0; this.addClass("bar-plot"); @@ -7722,16 +7721,6 @@ var Plottable; return this; } }; - Bar.prototype.displayLabelsOffBar = function (display) { - if (display === undefined) { - return this._displayLabelsOffBar; - } - else { - this._displayLabelsOffBar = display; - this.render(); - return this; - } - }; Bar.prototype.labelFormatter = function (formatter) { if (formatter == null) { return this._labelFormatter; @@ -7928,71 +7917,65 @@ var Plottable; var secondaryAttrAvailableSpace = _this._isVertical ? w : h; var tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; var showLabelOffBar = _this._isVertical ? (measurement.height > h) : (measurement.width > w); - if ((measurement.height <= h && measurement.width <= w) || _this._displayLabelsOffBar) { - var offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); - if (!positive) { - offset = offset * -1; - } - var getY = function () { - var addend = 0; - if (_this._isVertical) { - addend += offset; - if (showLabelOffBar && positive) { - addend += (offset - h); - } - if (showLabelOffBar && !positive) { - addend += measurement.height; - } - ; + var offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); + if (!positive) { + offset = offset * -1; + } + var getY = function () { + var addend = 0; + if (_this._isVertical) { + addend += offset; + if (showLabelOffBar && positive) { + addend += (offset - h); } - return baselineY + addend; - }; - var getX = function () { - var addend = 0; - if (!_this._isVertical) { - addend += offset; - if (showLabelOffBar && positive) { - addend += (offset - w - Bar._LABEL_HORIZONTAL_PADDING); - } - if (showLabelOffBar && !positive) { - addend += measurement.width; - } - ; + if (showLabelOffBar && !positive) { + addend += measurement.height; } - return baselineX + addend; - }; - var x = getX(); - var y = getY(); - var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - if (_this._displayLabelsOffBar) { - var labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; - g.classed(labelPositioningClassName, true); - } - else { - var color = attrToProjector["fill"](d, i, dataset); - var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); - g.classed(dark ? "dark-label" : "light-label", true); + ; } - var hideLabel = (x + measurement.width > _this.width() || (positive ? y + measurement.height : y + h) > _this.height()); - g.style("visibility", hideLabel ? "hidden" : "inherit"); - var xAlign; - var yAlign; - if (_this._isVertical) { - xAlign = "center"; - yAlign = positive ? "top" : "bottom"; - } - else { - xAlign = positive ? "left" : "right"; - yAlign = "center"; + return baselineY + addend; + }; + var getX = function () { + var addend = 0; + if (!_this._isVertical) { + addend += offset; + if (showLabelOffBar && positive) { + addend += (offset - w - Bar._LABEL_HORIZONTAL_PADDING); + } + if (showLabelOffBar && !positive) { + addend += measurement.width; + } + ; } - var writeOptions = { - selection: g, - xAlign: xAlign, - yAlign: yAlign, - textRotation: 0 - }; - writer.write(text, w, h, writeOptions); + return baselineX + addend; + }; + var x = getX(); + var y = getY(); + var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); + var labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; + g.classed(labelPositioningClassName, true); + var color = attrToProjector["fill"](d, i, dataset); + var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); + g.classed(dark ? "dark-label" : "light-label", true); + var hideLabel = (x + measurement.width > _this.width() || (positive ? y + measurement.height : y + h) > _this.height()); + g.style("visibility", hideLabel ? "hidden" : "inherit"); + var xAlign; + var yAlign; + if (_this._isVertical) { + xAlign = "center"; + yAlign = positive ? "top" : "bottom"; + } + else { + xAlign = positive ? "left" : "right"; + yAlign = "center"; } + var writeOptions = { + selection: g, + xAlign: xAlign, + yAlign: yAlign, + textRotation: 0 + }; + writer.write(text, w, h, writeOptions); return tooWide; }); return labelTooWide.some(function (d) { return d; }); diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 59aaa2c5dc..70f42418bb 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -23,7 +23,6 @@ export module Plots { protected _isVertical: boolean; private _labelFormatter: Formatter = Formatters.identity(); private _labelsEnabled = false; - private _displayLabelsOffBar = false; private _hideBarsIfAnyAreTooWide = true; private _labelConfig: Utils.Map; private _baselineValueProvider: () => (X|Y)[]; @@ -191,30 +190,6 @@ export module Plots { } } - /** - * Get whether to display labels off the bar - * - * @returns {boolean} Whether labels should be displayed off the bar - */ - public displayLabelsOffBar(): boolean; - - /** - * Sets whether labels are displayed off the bar - * - * @param {boolean} displayLabelsOffBar - * @return {Bar} The calling Bar Plot. - */ - public displayLabelsOffBar(display: boolean): Bar ; - public displayLabelsOffBar(display?: boolean): any { - if (display === undefined) { - return this._displayLabelsOffBar; - } else { - this._displayLabelsOffBar = display; - this.render(); - return this; - } - } - /** * Gets the Formatter for the labels. */ @@ -454,73 +429,69 @@ export module Plots { let secondaryAttrTextSpace = this._isVertical ? measurement.width : measurement.height; let secondaryAttrAvailableSpace = this._isVertical ? w : h; let tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - let showLabelOffBar = this._isVertical ? (measurement.height > h) : (measurement.width > w); - if ((measurement.height <= h && measurement.width <= w) || this._displayLabelsOffBar) { - let offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); - if (!positive) { offset = offset * -1; } - - let getY = () => { - let addend = 0; - if (this._isVertical) { - addend += offset; - if (showLabelOffBar && positive) { - addend += (offset - h); - } - if (showLabelOffBar && !positive) { - addend += measurement.height; - }; - } - return baselineY + addend; - }; - - let getX = () => { - let addend = 0; - if (!this._isVertical) { - addend += offset; - if (showLabelOffBar && positive) { - addend += (offset - w - Bar._LABEL_HORIZONTAL_PADDING); - } - if (showLabelOffBar && !positive) { - addend += measurement.width; - }; - } - return baselineX + addend; - }; - - let x = getX(); - let y = getY(); - - let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - - if (this._displayLabelsOffBar) { - let labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; - g.classed(labelPositioningClassName, true); - } else { - let color = attrToProjector["fill"](d, i, dataset); - let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); - g.classed(dark ? "dark-label" : "light-label", true); + let showLabelOffBar = this._isVertical ? (measurement.height > h) : (measurement.width > w) + let offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); + if (!positive) { offset = offset * -1; } + + let getY = () => { + let addend = 0; + if (this._isVertical) { + addend += offset; + if (showLabelOffBar && positive) { + addend += (offset - h); + } + if (showLabelOffBar && !positive) { + addend += measurement.height; + }; + } + return baselineY + addend; + }; + + let getX = () => { + let addend = 0; + if (!this._isVertical) { + addend += offset; + if (showLabelOffBar && positive) { + addend += (offset - w - Bar._LABEL_HORIZONTAL_PADDING); + } + if (showLabelOffBar && !positive) { + addend += measurement.width; + }; } + return baselineX + addend; + }; - let hideLabel = (x + measurement.width > this.width() || (positive ? y + measurement.height : y + h) > this.height()); + let x = getX(); + let y = getY(); - g.style("visibility", hideLabel ? "hidden" : "inherit"); - let xAlign: string; - let yAlign: string; - if (this._isVertical) { - xAlign = "center"; - yAlign = positive ? "top" : "bottom"; - } else { - xAlign = positive ? "left" : "right"; - yAlign = "center"; - } - let writeOptions = { - selection: g, - xAlign: xAlign, - yAlign: yAlign, - textRotation: 0 - }; - writer.write(text, w, h, writeOptions); + let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); + + let labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; + g.classed(labelPositioningClassName, true); + + let color = attrToProjector["fill"](d, i, dataset); + let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); + g.classed(dark ? "dark-label" : "light-label", true); + + let hideLabel = (x + measurement.width > this.width() || (positive ? y + measurement.height : y + h) > this.height()); + + g.style("visibility", hideLabel ? "hidden" : "inherit"); + let xAlign: string; + let yAlign: string; + if (this._isVertical) { + xAlign = "center"; + yAlign = positive ? "top" : "bottom"; + } else { + xAlign = positive ? "left" : "right"; + yAlign = "center"; } + let writeOptions = { + selection: g, + xAlign: xAlign, + yAlign: yAlign, + textRotation: 0 + }; + writer.write(text, w, h, writeOptions); return tooWide; }); return labelTooWide.some((d: boolean) => d); diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index b88fdc83d6..dccf8a9e62 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -635,7 +635,6 @@ describe("Plots", () => { }); it("shows both inner and outer labels", () => { - barPlot.displayLabelsOffBar(true); barPlot.renderTo(svg); let texts = svg.selectAll("text"); @@ -681,9 +680,10 @@ describe("Plots", () => { plot.renderTo(svg); plot.labelsEnabled(true); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 2, "both texts drawn"); - assert.strictEqual(texts[0], "640", "first label is 640"); - assert.strictEqual(texts[1], "12345", "first label is 12345"); + assert.lengthOf(texts, 3, "all texts drawn"); + assert.strictEqual(texts[0], "5", "first label is 5"); + assert.strictEqual(texts[1], "640", "first label is 640"); + assert.strictEqual(texts[2], "12345", "first label is 12345"); svg.remove(); }); @@ -701,14 +701,14 @@ describe("Plots", () => { plot.labelFormatter((n: number) => n.toString() + "%"); plot.renderTo(svg); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 2, "both texts drawn"); - assert.strictEqual(texts[0], "640%", "first label is 640%"); - assert.strictEqual(texts[1], "12345%", "first label is 12345%"); + assert.lengthOf(texts, 3, "all texts drawn"); + assert.strictEqual(texts[0], "5%", "first label is 5%"); + assert.strictEqual(texts[1], "640%", "first label is 640%"); + assert.strictEqual(texts[2], "12345%", "first label is 12345%"); svg.remove(); }); it("bar labels are shown inside or outside the bar as appropriate", () => { - plot.displayLabelsOffBar(true); plot.labelsEnabled(true); plot.renderTo(svg); @@ -727,14 +727,14 @@ describe("Plots", () => { plot.labelsEnabled(true); plot.renderTo(svg); let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 2, "both texts drawn"); + assert.lengthOf(texts, 3, "all texts drawn"); let originalDrawLabels = ( plot)._drawLabels; let called = false; ( plot)._drawLabels = () => { if (!called) { originalDrawLabels.apply(plot); texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 2, "texts were repopulated by drawLabels after the update"); + assert.lengthOf(texts, 3, "texts were repopulated by drawLabels after the update"); svg.remove(); called = true; // for some reason, in phantomJS, `done` was being called multiple times and this caused the test to fail. done(); From 42c4408da9a80f9493e04da0f858e93aab3f5699 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 19 Aug 2015 11:09:02 +0300 Subject: [PATCH 26/49] Rename baselineX/baselineY to baseX/baseY To avoid confusion with other uses of "baseline" --- plottable.js | 8 ++++---- src/plots/barPlot.ts | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/plottable.js b/plottable.js index 0fdb4e5c7d..d58351ab2e 100644 --- a/plottable.js +++ b/plottable.js @@ -7907,8 +7907,8 @@ var Plottable; var text = _this._labelFormatter(primaryAccessor(d, i, dataset)).toString(); var w = attrToProjector["width"](d, i, dataset); var h = attrToProjector["height"](d, i, dataset); - var baselineX = attrToProjector["x"](d, i, dataset); - var baselineY = attrToProjector["y"](d, i, dataset); + var baseX = attrToProjector["x"](d, i, dataset); + var baseY = attrToProjector["y"](d, i, dataset); var positive = originalPositionFn(d, i, dataset) <= scaledBaseline; var measurement = measurer.measure(text); var primary = _this._isVertical ? h : w; @@ -7933,7 +7933,7 @@ var Plottable; } ; } - return baselineY + addend; + return baseY + addend; }; var getX = function () { var addend = 0; @@ -7947,7 +7947,7 @@ var Plottable; } ; } - return baselineX + addend; + return baseX + addend; }; var x = getX(); var y = getY(); diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 70f42418bb..bbdfef7794 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -420,8 +420,8 @@ export module Plots { let text = this._labelFormatter(primaryAccessor(d, i, dataset)).toString(); let w = attrToProjector["width"](d, i, dataset); let h = attrToProjector["height"](d, i, dataset); - let baselineX = attrToProjector["x"](d, i, dataset); - let baselineY = attrToProjector["y"](d, i, dataset); + let baseX = attrToProjector["x"](d, i, dataset); + let baseY = attrToProjector["y"](d, i, dataset); let positive = originalPositionFn(d, i, dataset) <= scaledBaseline; let measurement = measurer.measure(text); let primary = this._isVertical ? h : w; @@ -444,7 +444,7 @@ export module Plots { addend += measurement.height; }; } - return baselineY + addend; + return baseY + addend; }; let getX = () => { @@ -458,7 +458,7 @@ export module Plots { addend += measurement.width; }; } - return baselineX + addend; + return baseX + addend; }; let x = getX(); From 6ae35a5b88aeec21eeb6e327952ceac9f1552086 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 19 Aug 2015 11:11:20 +0300 Subject: [PATCH 27/49] Add missing semicolon --- src/plots/barPlot.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index bbdfef7794..80634a45d8 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -429,7 +429,7 @@ export module Plots { let secondaryAttrTextSpace = this._isVertical ? measurement.width : measurement.height; let secondaryAttrAvailableSpace = this._isVertical ? w : h; let tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - let showLabelOffBar = this._isVertical ? (measurement.height > h) : (measurement.width > w) + let showLabelOffBar = this._isVertical ? (measurement.height > h) : (measurement.width > w); let offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); if (!positive) { offset = offset * -1; } From 793a7cc4f68486a674f20c8e8f7f1c663dc173bf Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Wed, 19 Aug 2015 11:37:50 +0300 Subject: [PATCH 28/49] Restore label hiding tests --- test/plots/barPlotTests.ts | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index dccf8a9e62..28b3f036a3 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -634,6 +634,36 @@ describe("Plots", () => { barPlot.renderTo(svg); }); + it("hides labels properly on the right", () => { + xScale.domainMax(0.95); + let texts = svg.selectAll("text"); + + assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); + + let label1 = d3.select(texts[0][0]); + let label2 = d3.select(texts[0][1]); + + assert.include(["visible", "inherit"], label1.style("visibility"), "label 1 is visible"); + assert.strictEqual(label2.style("visibility"), "hidden", "label 2 is not visible"); + + svg.remove(); + }); + + it("hides labels properly on the left", () => { + xScale.domainMax(-1.4); + let texts = svg.selectAll("text"); + + assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); + + let label1 = d3.select(texts[0][0]); + let label2 = d3.select(texts[0][1]); + + assert.strictEqual(label1.style("visibility"), "hidden", "label 2 is not visible"); + assert.include(["visible", "inherit"], label2.style("visibility"), "label 1 is visible"); + + svg.remove(); + }); + it("shows both inner and outer labels", () => { barPlot.renderTo(svg); From e1967a895110ec7901f86dbd40d5924abaeb0346 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Thu, 20 Aug 2015 11:22:14 +0300 Subject: [PATCH 29/49] Restore label hiding logic --- plottable.js | 23 +++++++++++++++++++++-- src/plots/barPlot.ts | 25 +++++++++++++++++++++++-- test/plots/barPlotTests.ts | 2 +- 3 files changed, 45 insertions(+), 5 deletions(-) diff --git a/plottable.js b/plottable.js index a95e0191ba..bf90637684 100644 --- a/plottable.js +++ b/plottable.js @@ -8283,8 +8283,27 @@ var Plottable; var color = attrToProjector["fill"](d, i, dataset); var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); g.classed(dark ? "dark-label" : "light-label", true); - var hideLabel = (x + measurement.width > _this.width() || (positive ? y + measurement.height : y + h) > _this.height()); - g.style("visibility", hideLabel ? "hidden" : "inherit"); + var showLabel = true; + var labelPosition = { + x: x, + y: positive ? y : y + h - measurement.height + }; + if (_this._isVertical) { + labelPosition.x = baseX + w / 2 - measurement.width / 2; + } + else { + if (!positive) { + labelPosition.x = baseX + offset + w - measurement.width; + } + else { + labelPosition.x = baseX + offset; + } + } + if (labelPosition.x < 0 || labelPosition.x + measurement.width > _this.width() || + labelPosition.y < 0 || labelPosition.y + measurement.height > _this.height()) { + showLabel = false; + } + g.style("visibility", showLabel ? "inherit" : "hidden"); var xAlign; var yAlign; if (_this._isVertical) { diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index b063c1ed36..5bf9215b00 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -502,9 +502,30 @@ export module Plots { let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); g.classed(dark ? "dark-label" : "light-label", true); - let hideLabel = (x + measurement.width > this.width() || (positive ? y + measurement.height : y + h) > this.height()); + let showLabel = true; + + let labelPosition = { + x: x, + y: positive ? y : y + h - measurement.height + }; + + if (this._isVertical) { + labelPosition.x = baseX + w / 2 - measurement.width / 2; + } else { + if (!positive) { + labelPosition.x = baseX + offset + w - measurement.width; + } else { + labelPosition.x = baseX + offset; + } + } + + if (labelPosition.x < 0 || labelPosition.x + measurement.width > this.width() || + labelPosition.y < 0 || labelPosition.y + measurement.height > this.height()) { + showLabel = false; + } + + g.style("visibility", showLabel ? "inherit" : "hidden"); - g.style("visibility", hideLabel ? "hidden" : "inherit"); let xAlign: string; let yAlign: string; if (this._isVertical) { diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 843684f9aa..1f1012ae0d 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -671,7 +671,7 @@ describe("Plots", () => { }); it("hides labels properly on the left", () => { - xScale.domainMax(-1.4); + xScale.domainMin(-1.4); let texts = svg.selectAll("text"); assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); From 9a2b76da2941d10da783c6da67105c6a7c05b2b2 Mon Sep 17 00:00:00 2001 From: Okal Otieno Date: Fri, 21 Aug 2015 10:11:04 +0300 Subject: [PATCH 30/49] Restore removed test --- test/plots/barPlotTests.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 1f1012ae0d..0f88236262 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -820,6 +820,23 @@ describe("Plots", () => { plot.renderTo(svg); }); + it("hides labels outside of the visible render area (horizontal)", () => { + xScale.domain([1, 3]); + + let texts = svg.selectAll("text"); + assert.strictEqual(texts.size(), plot.datasets()[0].data().length, "One label rendered for each piece of data"); + + let label1 = d3.select(texts[0][0]); + let label2 = d3.select(texts[0][1]); + let label3 = d3.select(texts[0][2]); + + assert.strictEqual(label1.style("visibility"), "hidden", "Left label is cut off by the margin"); + assert.include(["visible", "inherit"], label2.style("visibility"), "Middle label should still show"); + assert.strictEqual(label3.style("visibility"), "hidden", "Right label is cut off by the margin"); + + svg.remove(); + }); + it("hides labels outside of the visible render area (vertical)", () => { yScale.domain([2.5, 11]); From 509ece1ba963c839117216a4ca15eb85019f255b Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Wed, 16 Sep 2015 11:40:57 -0400 Subject: [PATCH 31/49] Fix DragBoxLayer tests. Interfered with debugging. --- test/components/dragBoxLayerTests.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/components/dragBoxLayerTests.ts b/test/components/dragBoxLayerTests.ts index 354afa70ae..a79375bac2 100644 --- a/test/components/dragBoxLayerTests.ts +++ b/test/components/dragBoxLayerTests.ts @@ -65,7 +65,7 @@ describe("Interactive Components", () => { it("can get and set the detection radius", () => { assert.strictEqual(dbl.detectionRadius(), 3, "there is a default detection radius"); assert.doesNotThrow(() => dbl.detectionRadius(4), Error, "can set detection radius before anchoring"); - dbl.renderTo("svg"); + dbl.renderTo(svg); assert.strictEqual(dbl.detectionRadius(), 4, "detection radius did not change upon rendering"); assert.strictEqual(dbl.detectionRadius(5), dbl, "setting the detection radius returns the drag box layer"); @@ -76,7 +76,7 @@ describe("Interactive Components", () => { }); it("applies the given detection radius property", () => { - dbl.renderTo("svg"); + dbl.renderTo(svg); let radius = 5; dbl.detectionRadius(radius); From fd1b139d2325c706b518aba8404579fb994237ea Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Wed, 16 Sep 2015 14:03:47 -0400 Subject: [PATCH 32/49] [Plots.Bar] Off-bar labels don't receive light/dark class --- plottable.js | 14 +++++++++----- src/plots/barPlot.ts | 14 ++++++++------ 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/plottable.js b/plottable.js index 6160d391c5..c7cc3b7d4d 100644 --- a/plottable.js +++ b/plottable.js @@ -8700,11 +8700,15 @@ var Plottable; var x = getX(); var y = getY(); var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - var labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; - g.classed(labelPositioningClassName, true); - var color = attrToProjector["fill"](d, i, dataset); - var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); - g.classed(dark ? "dark-label" : "light-label", true); + if (showLabelOffBar) { + g.classed("off-bar-label", true); + } + else { + g.classed("on-bar-label", true); + var color = attrToProjector["fill"](d, i, dataset); + var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); + g.classed(dark ? "dark-label" : "light-label", true); + } var showLabel = true; var labelPosition = { x: x, diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 2e45113452..3799a78245 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -522,12 +522,14 @@ export module Plots { let y = getY(); let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - let labelPositioningClassName = showLabelOffBar ? "off-bar-label" : "on-bar-label"; - g.classed(labelPositioningClassName, true); - - let color = attrToProjector["fill"](d, i, dataset); - let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); - g.classed(dark ? "dark-label" : "light-label", true); + if (showLabelOffBar) { + g.classed("off-bar-label", true); + } else { + g.classed("on-bar-label", true); + let color = attrToProjector["fill"](d, i, dataset); + let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); + g.classed(dark ? "dark-label" : "light-label", true); + } let showLabel = true; From 8cc177a902731423696bce71687e5c6ade630a69 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Fri, 18 Sep 2015 11:33:54 -0400 Subject: [PATCH 33/49] [Plots.Bar] Zero counts as positive (non-negative) for label drawing Redid the logic slightly, so zero is always on the "positive" side, regardless of the orientation of the Plots.Bar. Added tests to make sure zero displays properly. --- plottable.js | 24 +++++++++++------------- src/plots/barPlot.ts | 24 +++++++++++------------- test/plots/barPlotTests.ts | 32 +++++++++++++++++++++++++++++++- 3 files changed, 53 insertions(+), 27 deletions(-) diff --git a/plottable.js b/plottable.js index c7cc3b7d4d..1179f7f677 100644 --- a/plottable.js +++ b/plottable.js @@ -8649,9 +8649,6 @@ var Plottable; var writer = labelConfig.writer; var labelTooWide = data.map(function (d, i) { var primaryAccessor = _this._isVertical ? _this.y().accessor : _this.x().accessor; - var originalPositionFn = _this._isVertical ? Plottable.Plot._scaledAccessor(_this.y()) : Plottable.Plot._scaledAccessor(_this.x()); - var primaryScale = _this._isVertical ? _this.y().scale : _this.x().scale; - var scaledBaseline = primaryScale.scale(_this.baselineValue()); var text = _this._labelFormatter(primaryAccessor(d, i, dataset)).toString(); var w = attrToProjector["width"](d, i, dataset); var h = attrToProjector["height"](d, i, dataset); @@ -8665,18 +8662,19 @@ var Plottable; var tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; var showLabelOffBar = _this._isVertical ? (measurement.height > h) : (measurement.width > w); var offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); - var positive = originalPositionFn(d, i, dataset) <= scaledBaseline; - if (!positive) { + var valueIsNegative = primaryAccessor(d, i, dataset) < _this.baselineValue(); + var positiveShift = _this._isVertical ? !valueIsNegative : valueIsNegative; + if (!positiveShift) { offset = offset * -1; } var getY = function () { var addend = 0; if (_this._isVertical) { addend += offset; - if (showLabelOffBar && positive) { + if (showLabelOffBar && positiveShift) { addend += (offset - h); } - if (showLabelOffBar && !positive) { + if (showLabelOffBar && !positiveShift) { addend += measurement.height; } ; @@ -8687,10 +8685,10 @@ var Plottable; var addend = 0; if (!_this._isVertical) { addend += offset; - if (showLabelOffBar && positive) { + if (showLabelOffBar && positiveShift) { addend += (offset - w - Bar._LABEL_HORIZONTAL_PADDING); } - if (showLabelOffBar && !positive) { + if (showLabelOffBar && !positiveShift) { addend += measurement.width; } ; @@ -8712,14 +8710,14 @@ var Plottable; var showLabel = true; var labelPosition = { x: x, - y: positive ? y : y + h - measurement.height + y: positiveShift ? y : y + h - measurement.height }; if (_this._isVertical) { labelPosition.x = baseX + w / 2 - measurement.width / 2; } else { labelPosition.y = baseY + h / 2 - measurement.height / 2; - if (!positive) { + if (!positiveShift) { labelPosition.x = baseX + offset + w - measurement.width; } else { @@ -8735,10 +8733,10 @@ var Plottable; var yAlign; if (_this._isVertical) { xAlign = "center"; - yAlign = positive ? "top" : "bottom"; + yAlign = positiveShift ? "top" : "bottom"; } else { - xAlign = positive ? "left" : "right"; + xAlign = positiveShift ? "left" : "right"; yAlign = "center"; } var writeOptions = { diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 3799a78245..1d00bc43af 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -470,9 +470,6 @@ export module Plots { let writer = labelConfig.writer; let labelTooWide: boolean[] = data.map((d, i) => { let primaryAccessor = this._isVertical ? this.y().accessor : this.x().accessor; - let originalPositionFn = this._isVertical ? Plot._scaledAccessor(this.y()) : Plot._scaledAccessor(this.x()); - let primaryScale: Scale = this._isVertical ? this.y().scale : this.x().scale; - let scaledBaseline = primaryScale.scale(this.baselineValue()); let text = this._labelFormatter(primaryAccessor(d, i, dataset)).toString(); let w = attrToProjector["width"](d, i, dataset); let h = attrToProjector["height"](d, i, dataset); @@ -487,17 +484,18 @@ export module Plots { let showLabelOffBar = this._isVertical ? (measurement.height > h) : (measurement.width > w); let offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); - let positive = originalPositionFn(d, i, dataset) <= scaledBaseline; - if (!positive) { offset = offset * -1; } + let valueIsNegative = primaryAccessor(d, i, dataset) < this.baselineValue(); + let positiveShift = this._isVertical ? !valueIsNegative : valueIsNegative; + if (!positiveShift) { offset = offset * -1; } let getY = () => { let addend = 0; if (this._isVertical) { addend += offset; - if (showLabelOffBar && positive) { + if (showLabelOffBar && positiveShift) { addend += (offset - h); } - if (showLabelOffBar && !positive) { + if (showLabelOffBar && !positiveShift) { addend += measurement.height; }; } @@ -508,10 +506,10 @@ export module Plots { let addend = 0; if (!this._isVertical) { addend += offset; - if (showLabelOffBar && positive) { + if (showLabelOffBar && positiveShift) { addend += (offset - w - Bar._LABEL_HORIZONTAL_PADDING); } - if (showLabelOffBar && !positive) { + if (showLabelOffBar && !positiveShift) { addend += measurement.width; }; } @@ -535,14 +533,14 @@ export module Plots { let labelPosition = { x: x, - y: positive ? y : y + h - measurement.height + y: positiveShift ? y : y + h - measurement.height }; if (this._isVertical) { labelPosition.x = baseX + w / 2 - measurement.width / 2; } else { labelPosition.y = baseY + h / 2 - measurement.height / 2; - if (!positive) { + if (!positiveShift) { labelPosition.x = baseX + offset + w - measurement.width; } else { labelPosition.x = baseX + offset; @@ -560,9 +558,9 @@ export module Plots { let yAlign: string; if (this._isVertical) { xAlign = "center"; - yAlign = positive ? "top" : "bottom"; + yAlign = positiveShift ? "top" : "bottom"; } else { - xAlign = positive ? "left" : "right"; + xAlign = positiveShift ? "left" : "right"; yAlign = "center"; } let writeOptions = { diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index a3f1629a26..edbba56504 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -727,6 +727,7 @@ describe("Plots", () => { let yScale: Plottable.Scales.Linear; let xScale: Plottable.Scales.Linear; let barPlot: Plottable.Plots.Bar; + let dataset: Plottable.Dataset; beforeEach(() => { svg = TestMethods.generateSVG(600, 400); yScale = new Plottable.Scales.Linear(); @@ -738,7 +739,8 @@ describe("Plots", () => { ]; barPlot = new Plottable.Plots.Bar(Plottable.Plots.Bar.ORIENTATION_HORIZONTAL); - barPlot.addDataset(new Plottable.Dataset(data)); + dataset = new Plottable.Dataset(data); + barPlot.addDataset(dataset); barPlot.x((d) => d.x, xScale); barPlot.y((d) => d.y, yScale); barPlot.labelsEnabled(true); @@ -789,6 +791,20 @@ describe("Plots", () => { svg.remove(); }); + it("shows labels for bars with value = baseline on the \"positive\" side of the baseline", () => { + let zeroOnlyData = [ { x: 0, y: 0 }]; + dataset.data(zeroOnlyData); + barPlot.labelsEnabled(true); + barPlot.renderTo(svg); + + let labels = barPlot.content().selectAll("text"); + assert.strictEqual(labels.size(), 1, "one label drawn for data point"); + let labelPosition = ( labels.node()).getBoundingClientRect().left + window.Pixel_CloseTo_Requirement; + let linePosition = ( barPlot.content().select(".baseline").node()).getBoundingClientRect().right; + assert.operator(labelPosition, ">=", linePosition, "label with value=baseline is drawn to the right of the baseline"); + svg.remove(); + }); + it("hides labels that are partially cut off in y", () => { yScale.domain([1, 2]); let texts = barPlot.content().selectAll("text"); @@ -927,6 +943,20 @@ describe("Plots", () => { svg.remove(); }); + it("shows labels for bars with value = baseline on the \"positive\" side of the baseline", () => { + let zeroOnlyData = [ { x: "foo", y: 0 }]; + dataset.data(zeroOnlyData); + plot.labelsEnabled(true); + plot.renderTo(svg); + + let labels = plot.content().selectAll("text"); + assert.strictEqual(labels.size(), 1, "one label drawn for data point"); + let labelPosition = ( labels.node()).getBoundingClientRect().bottom - window.Pixel_CloseTo_Requirement; + let linePosition = ( plot.content().select(".baseline").node()).getBoundingClientRect().top; + assert.operator(labelPosition, "<=", linePosition, "label with value=baseline is drawn above the baseline"); + svg.remove(); + }); + it("bar labels are removed instantly on dataset change", (done) => { plot.labelsEnabled(true); plot.renderTo(svg); From ae1b0331d4b540cc83c045778948d41acd8e99c0 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Tue, 22 Sep 2015 08:54:24 -0700 Subject: [PATCH 34/49] [Plots.Bar] Refactor label-drawing logic. Structured into if-blocks for readability. Chose more descriptive variable names. --- plottable.js | 168 ++++++++++++++++++++------------------- src/plots/barPlot.ts | 185 +++++++++++++++++++++++-------------------- 2 files changed, 184 insertions(+), 169 deletions(-) diff --git a/plottable.js b/plottable.js index b014a7a11e..96a9c01085 100644 --- a/plottable.js +++ b/plottable.js @@ -8700,107 +8700,113 @@ var Plottable; var labelArea = labelConfig.labelArea; var measurer = labelConfig.measurer; var writer = labelConfig.writer; - var labelTooWide = data.map(function (d, i) { - var primaryAccessor = _this._isVertical ? _this.y().accessor : _this.x().accessor; - var text = _this._labelFormatter(primaryAccessor(d, i, dataset)).toString(); - var w = attrToProjector["width"](d, i, dataset); - var h = attrToProjector["height"](d, i, dataset); - var baseX = attrToProjector["x"](d, i, dataset); - var baseY = attrToProjector["y"](d, i, dataset); + var drawLabel = function (d, i) { + var valueAccessor = _this._isVertical ? _this.y().accessor : _this.x().accessor; + var value = valueAccessor(d, i, dataset); + var valueScale = _this._isVertical ? _this.y().scale : _this.x().scale; + var scaledValue = valueScale != null ? valueScale.scale(value) : value; + var scaledBaseline = valueScale != null ? valueScale.scale(_this.baselineValue()) : _this.baselineValue(); + var barWidth = attrToProjector["width"](d, i, dataset); + var barHeight = attrToProjector["height"](d, i, dataset); + var text = _this._labelFormatter(valueAccessor(d, i, dataset)).toString(); var measurement = measurer.measure(text); - var primary = _this._isVertical ? h : w; - var primarySpace = _this._isVertical ? measurement.height : measurement.width; - var secondaryAttrTextSpace = _this._isVertical ? measurement.width : measurement.height; - var secondaryAttrAvailableSpace = _this._isVertical ? w : h; - var tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - var showLabelOffBar = _this._isVertical ? (measurement.height > h) : (measurement.width > w); - var offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); - var valueIsNegative = primaryAccessor(d, i, dataset) < _this.baselineValue(); - var positiveShift = _this._isVertical ? !valueIsNegative : valueIsNegative; - if (!positiveShift) { - offset = offset * -1; - } - var getY = function () { - var addend = 0; - if (_this._isVertical) { - addend += offset; - if (showLabelOffBar && positiveShift) { - addend += (offset - h); + var xAlignment = "center"; + var yAlignment = "center"; + var labelContainerOrigin = { + x: attrToProjector["x"](d, i, dataset), + y: attrToProjector["y"](d, i, dataset) + }; + var containerWidth = barWidth; + var containerHeight = barHeight; + var labelOrigin = { + x: labelContainerOrigin.x, + y: labelContainerOrigin.y + }; + var showLabelOnBar = _this._isVertical ? (measurement.height <= barHeight) : (measurement.width <= barWidth); + if (_this._isVertical) { + labelOrigin.x += containerWidth / 2 - measurement.width / 2; + if (showLabelOnBar) { + var offset = Math.min((barHeight - measurement.height) / 2, Bar._LABEL_VERTICAL_PADDING); + if (scaledValue < scaledBaseline) { + labelContainerOrigin.y += offset; + yAlignment = "top"; + labelOrigin.y += offset; } - if (showLabelOffBar && !positiveShift) { - addend += measurement.height; + else { + labelContainerOrigin.y -= offset; + yAlignment = "bottom"; + labelOrigin.y += containerHeight - offset - measurement.height; } - ; } - return baseY + addend; - }; - var getX = function () { - var addend = 0; - if (!_this._isVertical) { - addend += offset; - if (showLabelOffBar && positiveShift) { - addend += (offset - w - Bar._LABEL_HORIZONTAL_PADDING); + else { + var offset = Bar._LABEL_VERTICAL_PADDING; + containerHeight = barHeight + offset + measurement.height; + if (scaledValue <= scaledBaseline) { + labelContainerOrigin.y -= offset + measurement.height; + yAlignment = "top"; + labelOrigin.y -= offset + measurement.height; } - if (showLabelOffBar && !positiveShift) { - addend += measurement.width; + else { + yAlignment = "bottom"; + labelOrigin.y += barHeight + offset; } - ; } - return baseX + addend; - }; - var x = getX(); - var y = getY(); - var g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); - if (showLabelOffBar) { - g.classed("off-bar-label", true); - } - else { - g.classed("on-bar-label", true); - var color = attrToProjector["fill"](d, i, dataset); - var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); - g.classed(dark ? "dark-label" : "light-label", true); - } - var showLabel = true; - var labelPosition = { - x: x, - y: positiveShift ? y : y + h - measurement.height - }; - if (_this._isVertical) { - labelPosition.x = baseX + w / 2 - measurement.width / 2; } else { - labelPosition.y = baseY + h / 2 - measurement.height / 2; - if (!positiveShift) { - labelPosition.x = baseX + offset + w - measurement.width; + labelOrigin.y += containerHeight / 2 - measurement.height / 2; + if (showLabelOnBar) { + var offset = Math.min((barWidth - measurement.width) / 2, Bar._LABEL_HORIZONTAL_PADDING); + if (scaledValue < scaledBaseline) { + labelContainerOrigin.x += offset; + xAlignment = "left"; + labelOrigin.x += offset; + } + else { + labelContainerOrigin.x -= offset; + xAlignment = "right"; + labelOrigin.x += containerWidth - offset - measurement.width; + } } else { - labelPosition.x = baseX + offset; + var offset = Bar._LABEL_HORIZONTAL_PADDING; + containerWidth = barWidth + offset + measurement.width; + if (scaledValue < scaledBaseline) { + labelContainerOrigin.x -= offset + measurement.width; + xAlignment = "left"; + labelOrigin.x -= offset + measurement.width; + } + else { + xAlignment = "right"; + labelOrigin.x += barWidth + offset; + } } } - if (labelPosition.x < 0 || labelPosition.x + measurement.width > _this.width() || - labelPosition.y < 0 || labelPosition.y + measurement.height > _this.height()) { - showLabel = false; - } - g.style("visibility", showLabel ? "inherit" : "hidden"); - var xAlign; - var yAlign; - if (_this._isVertical) { - xAlign = "center"; - yAlign = positiveShift ? "top" : "bottom"; + var labelContainer = labelArea.append("g").attr("transform", "translate(" + labelContainerOrigin.x + ", " + labelContainerOrigin.y + ")"); + if (showLabelOnBar) { + labelContainer.classed("on-bar-label", true); + var color = attrToProjector["fill"](d, i, dataset); + var dark = Plottable.Utils.Color.contrast("white", color) * 1.6 < Plottable.Utils.Color.contrast("black", color); + labelContainer.classed(dark ? "dark-label" : "light-label", true); } else { - xAlign = positiveShift ? "left" : "right"; - yAlign = "center"; + labelContainer.classed("off-bar-label", true); } + var hideLabel = labelOrigin.x < 0 || + labelOrigin.y < 0 || + labelOrigin.x + measurement.width > _this.width() || + labelOrigin.y + measurement.height > _this.height(); + labelContainer.style("visibility", hideLabel ? "hidden" : "inherit"); var writeOptions = { - selection: g, - xAlign: xAlign, - yAlign: yAlign, + selection: labelContainer, + xAlign: xAlignment, + yAlign: yAlignment, textRotation: 0 }; - writer.write(text, w, h, writeOptions); + writer.write(text, containerWidth, containerHeight, writeOptions); + var tooWide = _this._isVertical ? barWidth < measurement.width : barHeight < measurement.height; return tooWide; - }); + }; + var labelTooWide = data.map(drawLabel); return labelTooWide.some(function (d) { return d; }); }; Bar.prototype._generateDrawSteps = function () { diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 1d00bc43af..e66d15051e 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -468,110 +468,119 @@ export module Plots { let labelArea = labelConfig.labelArea; let measurer = labelConfig.measurer; let writer = labelConfig.writer; - let labelTooWide: boolean[] = data.map((d, i) => { - let primaryAccessor = this._isVertical ? this.y().accessor : this.x().accessor; - let text = this._labelFormatter(primaryAccessor(d, i, dataset)).toString(); - let w = attrToProjector["width"](d, i, dataset); - let h = attrToProjector["height"](d, i, dataset); - let baseX = attrToProjector["x"](d, i, dataset); - let baseY = attrToProjector["y"](d, i, dataset); - let measurement = measurer.measure(text); - let primary = this._isVertical ? h : w; - let primarySpace = this._isVertical ? measurement.height : measurement.width; - let secondaryAttrTextSpace = this._isVertical ? measurement.width : measurement.height; - let secondaryAttrAvailableSpace = this._isVertical ? w : h; - let tooWide = secondaryAttrTextSpace + 2 * Bar._LABEL_HORIZONTAL_PADDING > secondaryAttrAvailableSpace; - let showLabelOffBar = this._isVertical ? (measurement.height > h) : (measurement.width > w); - - let offset = Math.min((primary - primarySpace) / 2, Bar._LABEL_VERTICAL_PADDING); - let valueIsNegative = primaryAccessor(d, i, dataset) < this.baselineValue(); - let positiveShift = this._isVertical ? !valueIsNegative : valueIsNegative; - if (!positiveShift) { offset = offset * -1; } - - let getY = () => { - let addend = 0; - if (this._isVertical) { - addend += offset; - if (showLabelOffBar && positiveShift) { - addend += (offset - h); - } - if (showLabelOffBar && !positiveShift) { - addend += measurement.height; - }; - } - return baseY + addend; - }; - let getX = () => { - let addend = 0; - if (!this._isVertical) { - addend += offset; - if (showLabelOffBar && positiveShift) { - addend += (offset - w - Bar._LABEL_HORIZONTAL_PADDING); - } - if (showLabelOffBar && !positiveShift) { - addend += measurement.width; - }; - } - return baseX + addend; - }; - - let x = getX(); - let y = getY(); - let g = labelArea.append("g").attr("transform", "translate(" + x + "," + y + ")"); + let drawLabel = (d: any, i: number) => { + let valueAccessor = this._isVertical ? this.y().accessor : this.x().accessor; + let value = valueAccessor(d, i, dataset); + let valueScale: Scale = this._isVertical ? this.y().scale : this.x().scale; + let scaledValue = valueScale != null ? valueScale.scale(value) : value; + let scaledBaseline = valueScale != null ? valueScale.scale(this.baselineValue()) : this.baselineValue(); - if (showLabelOffBar) { - g.classed("off-bar-label", true); - } else { - g.classed("on-bar-label", true); - let color = attrToProjector["fill"](d, i, dataset); - let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); - g.classed(dark ? "dark-label" : "light-label", true); - } + let barWidth = attrToProjector["width"](d, i, dataset); + let barHeight = attrToProjector["height"](d, i, dataset); + let text = this._labelFormatter(valueAccessor(d, i, dataset)).toString(); + let measurement = measurer.measure(text); - let showLabel = true; + let xAlignment = "center"; + let yAlignment = "center"; + let labelContainerOrigin = { + x: attrToProjector["x"](d, i, dataset), + y: attrToProjector["y"](d, i, dataset) + }; + let containerWidth = barWidth; + let containerHeight = barHeight; - let labelPosition = { - x: x, - y: positiveShift ? y : y + h - measurement.height + let labelOrigin = { + x: labelContainerOrigin.x, + y: labelContainerOrigin.y }; + let showLabelOnBar = this._isVertical ? (measurement.height <= barHeight) : (measurement.width <= barWidth); + if (this._isVertical) { - labelPosition.x = baseX + w / 2 - measurement.width / 2; - } else { - labelPosition.y = baseY + h / 2 - measurement.height / 2; - if (!positiveShift) { - labelPosition.x = baseX + offset + w - measurement.width; - } else { - labelPosition.x = baseX + offset; + labelOrigin.x += containerWidth / 2 - measurement.width / 2; + + if (showLabelOnBar) { + let offset = Math.min( (barHeight - measurement.height ) / 2, Bar._LABEL_VERTICAL_PADDING); + if (scaledValue < scaledBaseline) { + labelContainerOrigin.y += offset; + yAlignment = "top"; + labelOrigin.y += offset; + } else { + labelContainerOrigin.y -= offset; + yAlignment = "bottom"; + labelOrigin.y += containerHeight - offset - measurement.height; + } + } else { // show label off bar + let offset = Bar._LABEL_VERTICAL_PADDING; + containerHeight = barHeight + offset + measurement.height; + if (scaledValue <= scaledBaseline) { + labelContainerOrigin.y -= offset + measurement.height; + yAlignment = "top"; + labelOrigin.y -= offset + measurement.height; + } else { + yAlignment = "bottom"; + labelOrigin.y += barHeight + offset; + } + } + } else { // horizontal + labelOrigin.y += containerHeight / 2 - measurement.height / 2; + + if (showLabelOnBar) { + let offset = Math.min( (barWidth - measurement.width ) / 2, Bar._LABEL_HORIZONTAL_PADDING); + if (scaledValue < scaledBaseline) { + labelContainerOrigin.x += offset; + xAlignment = "left"; + labelOrigin.x += offset; + } else { + labelContainerOrigin.x -= offset; + xAlignment = "right"; + labelOrigin.x += containerWidth - offset - measurement.width; + } + } else { // show label off bar + let offset = Bar._LABEL_HORIZONTAL_PADDING; + containerWidth = barWidth + offset + measurement.width; + if (scaledValue < scaledBaseline) { + labelContainerOrigin.x -= offset + measurement.width; + xAlignment = "left"; + labelOrigin.x -= offset + measurement.width; + } else { + xAlignment = "right"; + labelOrigin.x += barWidth + offset; + } } } - if (labelPosition.x < 0 || labelPosition.x + measurement.width > this.width() || - labelPosition.y < 0 || labelPosition.y + measurement.height > this.height()) { - showLabel = false; - } - - g.style("visibility", showLabel ? "inherit" : "hidden"); + let labelContainer = labelArea.append("g").attr("transform", `translate(${labelContainerOrigin.x}, ${labelContainerOrigin.y})`); - let xAlign: string; - let yAlign: string; - if (this._isVertical) { - xAlign = "center"; - yAlign = positiveShift ? "top" : "bottom"; + if (showLabelOnBar) { + labelContainer.classed("on-bar-label", true); + let color = attrToProjector["fill"](d, i, dataset); + let dark = Utils.Color.contrast("white", color) * 1.6 < Utils.Color.contrast("black", color); + labelContainer.classed(dark ? "dark-label" : "light-label", true); } else { - xAlign = positiveShift ? "left" : "right"; - yAlign = "center"; + labelContainer.classed("off-bar-label", true); } + + let hideLabel = labelOrigin.x < 0 || + labelOrigin.y < 0 || + labelOrigin.x + measurement.width > this.width() || + labelOrigin.y + measurement.height > this.height(); + labelContainer.style("visibility", hideLabel ? "hidden" : "inherit"); + let writeOptions = { - selection: g, - xAlign: xAlign, - yAlign: yAlign, - textRotation: 0 + selection: labelContainer, + xAlign: xAlignment, + yAlign: yAlignment, + textRotation: 0 }; - writer.write(text, w, h, writeOptions); + writer.write(text, containerWidth, containerHeight, writeOptions); + + let tooWide = this._isVertical ? barWidth < measurement.width : barHeight < measurement.height; return tooWide; - }); + }; + + let labelTooWide = data.map(drawLabel); return labelTooWide.some((d: boolean) => d); } From dee3994d2170a7b0526a187079760b974bb8f15d Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Tue, 22 Sep 2015 10:39:54 -0700 Subject: [PATCH 35/49] [Plots.Bar] Move basic label test. --- test/plots/barPlotTests.ts | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index edbba56504..e4533e3520 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -747,6 +747,20 @@ describe("Plots", () => { barPlot.renderTo(svg); }); + it("shows both inner and outer labels", () => { + barPlot.renderTo(svg); + + let texts = svg.selectAll("text"); + assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); + + let offBarLabelCount = d3.selectAll(".off-bar-label")[0].length; + assert.strictEqual(offBarLabelCount, 1, "There should be 1 labels rendered outside the bar"); + + let onBarLabelCount = d3.selectAll(".on-bar-label")[0].length; + assert.strictEqual(onBarLabelCount, 1, "There should be 1 labels rendered inside the bar"); + svg.remove(); + }); + it("hides labels properly on the right", () => { xScale.domainMax(0.95); let texts = svg.selectAll("text"); @@ -777,20 +791,6 @@ describe("Plots", () => { svg.remove(); }); - it("shows both inner and outer labels", () => { - barPlot.renderTo(svg); - - let texts = svg.selectAll("text"); - assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); - - let offBarLabelCount = d3.selectAll(".off-bar-label")[0].length; - assert.strictEqual(offBarLabelCount, 1, "There should be 1 labels rendered outside the bar"); - - let onBarLabelCount = d3.selectAll(".on-bar-label")[0].length; - assert.strictEqual(onBarLabelCount, 1, "There should be 1 labels rendered inside the bar"); - svg.remove(); - }); - it("shows labels for bars with value = baseline on the \"positive\" side of the baseline", () => { let zeroOnlyData = [ { x: 0, y: 0 }]; dataset.data(zeroOnlyData); From 8786e3a970ddb56a274f835bfc8c0babdb7af957 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Tue, 22 Sep 2015 14:03:14 -0700 Subject: [PATCH 36/49] [Plots.Bar] Revamp test for cut off labels on Horizontal Plots.Bar Previous testing didn't cover enough cases. New test data covers: * positive on-bar label * positive off-bar label * zero bar * negative off-bar label * negative on-bar label ... and also tests labels being cut off by all four sides of the Plot. --- test/plots/barPlotTests.ts | 112 ++++++++++++++++++++++--------------- 1 file changed, 67 insertions(+), 45 deletions(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index e4533e3520..8da2dbffb2 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -726,16 +726,33 @@ describe("Plots", () => { let svg: d3.Selection; let yScale: Plottable.Scales.Linear; let xScale: Plottable.Scales.Linear; + let DEFAULT_DOMAIN = [-5, 5]; let barPlot: Plottable.Plots.Bar; let dataset: Plottable.Dataset; + + let getCenterOfText = (textNode: SVGElement) => { + let plotBoundingClientRect = ( barPlot.background().node()).getBoundingClientRect(); + let labelBoundingClientRect = textNode.getBoundingClientRect(); + + return { + x: (labelBoundingClientRect.left + labelBoundingClientRect.right) / 2 - plotBoundingClientRect.left, + y: (labelBoundingClientRect.top + labelBoundingClientRect.bottom) / 2 - plotBoundingClientRect.top + }; + }; + beforeEach(() => { - svg = TestMethods.generateSVG(600, 400); + svg = TestMethods.generateSVG(); yScale = new Plottable.Scales.Linear(); + yScale.domain(DEFAULT_DOMAIN); xScale = new Plottable.Scales.Linear(); + xScale.domain(DEFAULT_DOMAIN); let data = [ - {y: 1, x: -1.5}, - {y: 2, x: 100} + { y: -4, x: -4 }, + { y: -2, x: -0.1}, + { y: 0, x: 0 }, + { y: 2, x: 0.1 }, + { y: 4, x: 4 } ]; barPlot = new Plottable.Plots.Bar(Plottable.Plots.Bar.ORIENTATION_HORIZONTAL); @@ -750,49 +767,71 @@ describe("Plots", () => { it("shows both inner and outer labels", () => { barPlot.renderTo(svg); - let texts = svg.selectAll("text"); - assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); + let texts = barPlot.content().selectAll("text"); + assert.strictEqual(texts.size(), dataset.data().length, "one label drawn per datum"); - let offBarLabelCount = d3.selectAll(".off-bar-label")[0].length; - assert.strictEqual(offBarLabelCount, 1, "There should be 1 labels rendered outside the bar"); + let offBarLabels = barPlot.content().selectAll(".off-bar-label"); + assert.strictEqual(offBarLabels.size(), 3, "there are 3 off-bar labels"); - let onBarLabelCount = d3.selectAll(".on-bar-label")[0].length; - assert.strictEqual(onBarLabelCount, 1, "There should be 1 labels rendered inside the bar"); + let onBarLabels = barPlot.content().selectAll(".on-bar-label"); + assert.strictEqual(onBarLabels.size(), 2, "there are 2 on-bar labels"); svg.remove(); }); - it("hides labels properly on the right", () => { - xScale.domainMax(0.95); - let texts = svg.selectAll("text"); - - assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); + it("hides labels cut off by the right edge", () => { + dataset.data().forEach((d, i) => { + let texts = svg.selectAll("text"); + let centerOfText = getCenterOfText( texts[0][i]); + let centerXValue = xScale.invert(centerOfText.x); + xScale.domain([centerXValue - (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0]), centerXValue]); - let label1 = d3.select(texts[0][0]); - let label2 = d3.select(texts[0][1]); + texts = svg.selectAll("text"); // re-select after rendering + assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + }) + svg.remove(); + }); - assert.include(["visible", "inherit"], label1.style("visibility"), "label 1 is visible"); - assert.strictEqual(label2.style("visibility"), "hidden", "label 2 is not visible"); + it("hides labels cut off by the left edge", () => { + dataset.data().forEach((d, i) => { + let texts = svg.selectAll("text"); + let centerOfText = getCenterOfText( texts[0][i]); + let centerXValue = xScale.invert(centerOfText.x); + xScale.domain([centerXValue, centerXValue + (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0])]); + texts = svg.selectAll("text"); // re-select after rendering + assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + }); svg.remove(); }); - it("hides labels properly on the left", () => { - xScale.domainMin(-1.4); - let texts = svg.selectAll("text"); - - assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); + it("hides labels cut off by the top edge", () => { + dataset.data().forEach((d, i) => { + let texts = svg.selectAll("text"); + let centerOfText = getCenterOfText( texts[0][i]); + let centerYValue = yScale.invert(centerOfText.y); + yScale.domain([centerYValue - (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0]), centerYValue]); - let label1 = d3.select(texts[0][0]); - let label2 = d3.select(texts[0][1]); + texts = svg.selectAll("text"); // re-select after rendering + assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + }) + svg.remove(); + }); - assert.strictEqual(label1.style("visibility"), "hidden", "label 2 is not visible"); - assert.include(["visible", "inherit"], label2.style("visibility"), "label 1 is visible"); + it("hides labels cut off by the bottom edge", () => { + dataset.data().forEach((d, i) => { + let texts = svg.selectAll("text"); + let centerOfText = getCenterOfText( texts[0][i]); + let centerYValue = yScale.invert(centerOfText.y); + yScale.domain([centerYValue, centerYValue + (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0])]); + texts = svg.selectAll("text"); // re-select after rendering + assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + }) svg.remove(); }); it("shows labels for bars with value = baseline on the \"positive\" side of the baseline", () => { - let zeroOnlyData = [ { x: 0, y: 0 }]; + let zeroOnlyData = [ { x: 0, y: 0 } ]; dataset.data(zeroOnlyData); barPlot.labelsEnabled(true); barPlot.renderTo(svg); @@ -804,23 +843,6 @@ describe("Plots", () => { assert.operator(labelPosition, ">=", linePosition, "label with value=baseline is drawn to the right of the baseline"); svg.remove(); }); - - it("hides labels that are partially cut off in y", () => { - yScale.domain([1, 2]); - let texts = barPlot.content().selectAll("text"); - - assert.strictEqual(texts.size(), 2, "There should be two labels rendered"); - - texts.each(function(d, i) { - let textBounding = ( this).getBoundingClientRect(); - let svgBounding = ( barPlot.background().node()).getBoundingClientRect(); - let isLabelCutOff = (textBounding.top < svgBounding.top && textBounding.bottom > svgBounding.top) - || (textBounding.top < svgBounding.bottom && textBounding.bottom > svgBounding.bottom); - assert.isTrue(isLabelCutOff, `label ${i} is partially cut off`); - assert.strictEqual(d3.select(this).style("visibility"), "hidden", `label ${i} is not visible`); - }); - svg.remove(); - }); }); describe("Horizontal Bar Plot extent calculation", () => { From d05c56cca21961a666ade8e54a05400572273679 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Tue, 22 Sep 2015 14:24:25 -0700 Subject: [PATCH 37/49] [Plots.Bar] Restructure vertical label tests to use better data Also now less fragile to data change. --- test/plots/barPlotTests.ts | 143 ++++++++++++++++++++++--------------- 1 file changed, 84 insertions(+), 59 deletions(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 8da2dbffb2..f35e7d657e 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -892,113 +892,138 @@ describe("Plots", () => { }); describe("Vertical Bar Plot With Bar Labels", () => { - let plot: Plottable.Plots.Bar; - let data: any[]; - let dataset: Plottable.Dataset; - let xScale: Plottable.Scales.Category; - let yScale: Plottable.Scales.Linear; let svg: d3.Selection; + let yScale: Plottable.Scales.Linear; + let xScale: Plottable.Scales.Linear; + let DEFAULT_DOMAIN = [-5, 5]; + let barPlot: Plottable.Plots.Bar; + let dataset: Plottable.Dataset; + + let getCenterOfText = (textNode: SVGElement) => { + let plotBoundingClientRect = ( barPlot.background().node()).getBoundingClientRect(); + let labelBoundingClientRect = textNode.getBoundingClientRect(); + + return { + x: (labelBoundingClientRect.left + labelBoundingClientRect.right) / 2 - plotBoundingClientRect.left, + y: (labelBoundingClientRect.top + labelBoundingClientRect.bottom) / 2 - plotBoundingClientRect.top + }; + }; beforeEach(() => { svg = TestMethods.generateSVG(); - data = [{x: "foo", y: 5}, {x: "bar", y: 640}, {x: "zoo", y: 12345}]; - xScale = new Plottable.Scales.Category(); yScale = new Plottable.Scales.Linear(); + yScale.domain(DEFAULT_DOMAIN); + xScale = new Plottable.Scales.Linear(); + xScale.domain(DEFAULT_DOMAIN); + + let data = [ + { x: -4, y: -4 }, + { x: -2, y: -0.1}, + { x: 0, y: 0 }, + { x: 2, y: 0.1 }, + { x: 4, y: 4 } + ]; + + barPlot = new Plottable.Plots.Bar(Plottable.Plots.Bar.ORIENTATION_VERTICAL); dataset = new Plottable.Dataset(data); - plot = new Plottable.Plots.Bar(); - plot.addDataset(dataset); - plot.x((d) => d.x, xScale); - plot.y((d) => d.y, yScale); + barPlot.addDataset(dataset); + barPlot.x((d) => d.x, xScale); + barPlot.y((d) => d.y, yScale); + barPlot.renderTo(svg); }); + it("bar labels disabled by default", () => { - plot.renderTo(svg); - let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 0, "by default, no texts are drawn"); + barPlot.renderTo(svg); + let texts = barPlot.content().selectAll("text"); + assert.strictEqual(texts.size(), 0, "by default, no texts are drawn"); svg.remove(); }); it("bar labels render properly", () => { - plot.renderTo(svg); - plot.labelsEnabled(true); - let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 3, "all texts drawn"); - assert.strictEqual(texts[0], "5", "first label is 5"); - assert.strictEqual(texts[1], "640", "first label is 640"); - assert.strictEqual(texts[2], "12345", "first label is 12345"); + barPlot.renderTo(svg); + barPlot.labelsEnabled(true); + let texts = barPlot.content().selectAll("text"); + let data = dataset.data(); + assert.strictEqual(texts.size(), data.length, "one label drawn per datum"); + texts.each(function(d, i) { + assert.strictEqual(d3.select(this).text(), data[i].y.toString(), `by default, label text is the bar's value (index ${i})`); + }); svg.remove(); }); it("bar labels hide if bars too skinny", () => { - plot.labelsEnabled(true); - plot.renderTo(svg); - plot.labelFormatter((n: number) => n.toString() + (n === 12345 ? "looong" : "")); - let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 0, "no text drawn"); + svg.attr("width", 100); + barPlot.labelsEnabled(true); + barPlot.renderTo(svg); + let texts = barPlot.content().selectAll("text"); + assert.strictEqual(texts.size(), 0, "no labels drawn"); svg.remove(); }); it("formatters are used properly", () => { - plot.labelsEnabled(true); - plot.labelFormatter((n: number) => n.toString() + "%"); - plot.renderTo(svg); - let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 3, "all texts drawn"); - assert.strictEqual(texts[0], "5%", "first label is 5%"); - assert.strictEqual(texts[1], "640%", "first label is 640%"); - assert.strictEqual(texts[2], "12345%", "first label is 12345%"); + barPlot.labelsEnabled(true); + let formatter = (n: number) => n.toString() + "%"; + barPlot.labelFormatter(formatter); + barPlot.renderTo(svg); + let texts = barPlot.content().selectAll("text"); + assert.strictEqual(texts.size(), dataset.data().length, "one label drawn per datum"); + let expectedTexts = dataset.data().map((d) => formatter(d.y)); + texts.each(function(d, i) { + assert.strictEqual(d3.select(this).text(), expectedTexts[i], `formatter is applied to the displayed value (index ${i})`); + }); svg.remove(); }); it("bar labels are shown inside or outside the bar as appropriate", () => { - plot.labelsEnabled(true); - plot.renderTo(svg); + barPlot.labelsEnabled(true); + barPlot.renderTo(svg); - let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 3, "both texts drawn"); + let texts = barPlot.content().selectAll("text"); + assert.strictEqual(texts.size(), dataset.data().length, "one label drawn per datum"); - let offBarLabelCount = d3.selectAll(".off-bar-label")[0].length; - assert.strictEqual(offBarLabelCount, 1, "There should be 1 label rendered outside the bar"); + let offBarLabels = barPlot.content().selectAll(".off-bar-label"); + assert.strictEqual(offBarLabels.size(), 3, "there are 3 off-bar labels"); - let onBarLabelCount = d3.selectAll(".on-bar-label")[0].length; - assert.strictEqual(onBarLabelCount, 2, "There should be 2 labels rendered inside the bar"); + let onBarLabels = barPlot.content().selectAll(".on-bar-label"); + assert.strictEqual(onBarLabels.size(), 2, "there are 2 on-bar labels"); svg.remove(); }); it("shows labels for bars with value = baseline on the \"positive\" side of the baseline", () => { - let zeroOnlyData = [ { x: "foo", y: 0 }]; + let zeroOnlyData = [ { x: 0, y: 0 }]; dataset.data(zeroOnlyData); - plot.labelsEnabled(true); - plot.renderTo(svg); + barPlot.labelsEnabled(true); + barPlot.renderTo(svg); - let labels = plot.content().selectAll("text"); + let labels = barPlot.content().selectAll("text"); assert.strictEqual(labels.size(), 1, "one label drawn for data point"); let labelPosition = ( labels.node()).getBoundingClientRect().bottom - window.Pixel_CloseTo_Requirement; - let linePosition = ( plot.content().select(".baseline").node()).getBoundingClientRect().top; + let linePosition = ( barPlot.content().select(".baseline").node()).getBoundingClientRect().top; assert.operator(labelPosition, "<=", linePosition, "label with value=baseline is drawn above the baseline"); svg.remove(); }); it("bar labels are removed instantly on dataset change", (done) => { - plot.labelsEnabled(true); - plot.renderTo(svg); - let texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 3, "all texts drawn"); - let originalDrawLabels = ( plot)._drawLabels; + barPlot.labelsEnabled(true); + barPlot.renderTo(svg); + let texts = barPlot.content().selectAll("text"); + assert.strictEqual(texts.size(), dataset.data().length, "one label drawn per datum"); + let originalDrawLabels = ( barPlot)._drawLabels; let called = false; - ( plot)._drawLabels = () => { + ( barPlot)._drawLabels = () => { if (!called) { - originalDrawLabels.apply(plot); - texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 3, "texts were repopulated by drawLabels after the update"); + originalDrawLabels.apply(barPlot); + texts = barPlot.content().selectAll("text"); + assert.strictEqual(texts.size(), dataset.data().length, "texts were repopulated by drawLabels after the update"); svg.remove(); called = true; // for some reason, in phantomJS, `done` was being called multiple times and this caused the test to fail. done(); } }; - dataset.data(data); - texts = svg.selectAll("text")[0].map((n: any) => d3.select(n).text()); - assert.lengthOf(texts, 0, "texts were immediately removed"); + dataset.data(dataset.data()); + texts = barPlot.content().selectAll("text"); + assert.strictEqual(texts.size(), 0, "texts were immediately removed"); }); }); From 02068add53114fef25116713b915d7aa47736f62 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Tue, 22 Sep 2015 14:30:50 -0700 Subject: [PATCH 38/49] [Plots.Bar] Add comprehensive tests for cut-off labels on Vertical Plots.Bar --- test/plots/barPlotTests.ts | 94 +++++++++++++++++++------------------- 1 file changed, 46 insertions(+), 48 deletions(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index f35e7d657e..097cf5e890 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -787,7 +787,7 @@ describe("Plots", () => { texts = svg.selectAll("text"); // re-select after rendering assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); - }) + }); svg.remove(); }); @@ -813,7 +813,7 @@ describe("Plots", () => { texts = svg.selectAll("text"); // re-select after rendering assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); - }) + }); svg.remove(); }); @@ -826,7 +826,7 @@ describe("Plots", () => { texts = svg.selectAll("text"); // re-select after rendering assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); - }) + }); svg.remove(); }); @@ -932,7 +932,6 @@ describe("Plots", () => { barPlot.renderTo(svg); }); - it("bar labels disabled by default", () => { barPlot.renderTo(svg); let texts = barPlot.content().selectAll("text"); @@ -1025,61 +1024,60 @@ describe("Plots", () => { texts = barPlot.content().selectAll("text"); assert.strictEqual(texts.size(), 0, "texts were immediately removed"); }); - }); - describe("Vertical Bar Plot label visibility", () => { - let svg: d3.Selection; - let plot: Plottable.Plots.Bar; - let xScale: Plottable.Scales.Linear; - let yScale: Plottable.Scales.Linear; + it("hides labels cut off by the right edge", () => { + barPlot.labelsEnabled(true); + dataset.data().forEach((d, i) => { + let texts = svg.selectAll("text"); + let centerOfText = getCenterOfText( texts[0][i]); + let centerXValue = xScale.invert(centerOfText.x); + xScale.domain([centerXValue - (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0]), centerXValue]); - beforeEach(() => { - svg = TestMethods.generateSVG(); - xScale = new Plottable.Scales.Linear(); - yScale = new Plottable.Scales.Linear(); - let data = [ - { x: 1, y: 10.1 }, - { x: 2, y: 5.3 }, - { x: 3, y: 2.8 } - ]; - plot = new Plottable.Plots.Bar(); - plot.x((d) => d.x, xScale); - plot.y((d) => d.y, yScale); - plot.addDataset(new Plottable.Dataset(data)); - plot.labelsEnabled(true); - plot.renderTo(svg); + texts = svg.selectAll("text"); // re-select after rendering + assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + }); + svg.remove(); }); - it("hides labels outside of the visible render area (horizontal)", () => { - xScale.domain([1, 3]); - - let texts = svg.selectAll("text"); - assert.strictEqual(texts.size(), plot.datasets()[0].data().length, "One label rendered for each piece of data"); - - let label1 = d3.select(texts[0][0]); - let label2 = d3.select(texts[0][1]); - let label3 = d3.select(texts[0][2]); - - assert.strictEqual(label1.style("visibility"), "hidden", "Left label is cut off by the margin"); - assert.include(["visible", "inherit"], label2.style("visibility"), "Middle label should still show"); - assert.strictEqual(label3.style("visibility"), "hidden", "Right label is cut off by the margin"); + it("hides labels cut off by the left edge", () => { + barPlot.labelsEnabled(true); + dataset.data().forEach((d, i) => { + let texts = svg.selectAll("text"); + let centerOfText = getCenterOfText( texts[0][i]); + let centerXValue = xScale.invert(centerOfText.x); + xScale.domain([centerXValue, centerXValue + (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0])]); + texts = svg.selectAll("text"); // re-select after rendering + assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + }); svg.remove(); }); - it("hides labels outside of the visible render area (vertical)", () => { - yScale.domain([2.5, 11]); + it("hides labels cut off by the top edge", () => { + barPlot.labelsEnabled(true); + dataset.data().forEach((d, i) => { + let texts = svg.selectAll("text"); + let centerOfText = getCenterOfText( texts[0][i]); + let centerYValue = yScale.invert(centerOfText.y); + yScale.domain([centerYValue - (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0]), centerYValue]); - let texts = svg.selectAll("text"); - assert.strictEqual(texts.size(), plot.datasets()[0].data().length, "One label rendered for each piece of data"); + texts = svg.selectAll("text"); // re-select after rendering + assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + }); + svg.remove(); + }); - let label1 = d3.select(texts[0][0]); - let label2 = d3.select(texts[0][1]); - let label3 = d3.select(texts[0][2]); + it("hides labels cut off by the bottom edge", () => { + barPlot.labelsEnabled(true); + dataset.data().forEach((d, i) => { + let texts = svg.selectAll("text"); + let centerOfText = getCenterOfText( texts[0][i]); + let centerYValue = yScale.invert(centerOfText.y); + yScale.domain([centerYValue, centerYValue + (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0])]); - assert.include(["visible", "inherit"], label1.style("visibility"), "Left label should still show"); - assert.include(["visible", "inherit"], label2.style("visibility"), "Middle label should still show"); - assert.strictEqual(label3.style("visibility"), "hidden", "Right label is cut off. bar is too short"); + texts = svg.selectAll("text"); // re-select after rendering + assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + }); svg.remove(); }); }); From 716dd3adfd39471305e27d0b93f9bd3589f396ac Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Tue, 22 Sep 2015 15:59:46 -0700 Subject: [PATCH 39/49] Add (failing) tests asserting StackedBar doesn't draw off-bar labels. --- test/plots/stackedBarPlotTests.ts | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/plots/stackedBarPlotTests.ts b/test/plots/stackedBarPlotTests.ts index 77a74c7e50..66de80fa9a 100644 --- a/test/plots/stackedBarPlotTests.ts +++ b/test/plots/stackedBarPlotTests.ts @@ -90,6 +90,20 @@ describe("Plots", () => { svg.remove(); }); + // HACKHACK #2795: correct off-bar label logic to be implemented + it("doesn't show any off-bar labels", () => { + renderer.labelsEnabled(true); + yScale.domain([0, 30]); + let offBarLabels = renderer.content().selectAll(".off-bar-label"); + assert.operator(offBarLabels.size(), ">", 0, "some off-bar labels are drawn"); + let offBarLabelVisibilities: string[] = []; + offBarLabels.each(function() { + offBarLabelVisibilities.push(d3.select(this).style("visibility")); + }); + assert.isTrue(offBarLabelVisibilities.every((visibility) => visibility === "hidden"), "all off-bar labels are hidden"); + svg.remove(); + }); + it("considers lying within a bar's y-range to mean it is closest", () => { let bars = ( renderer)._renderArea.selectAll("rect"); @@ -377,6 +391,20 @@ describe("Plots", () => { assert.closeTo(TestMethods.numAttr(bar3, "x"), rendererWidth / 3, 0.01, "x is correct for bar3"); svg.remove(); }); + + // HACKHACK #2795: correct off-bar label logic to be implemented + it("doesn't show any off-bar labels", () => { + renderer.labelsEnabled(true); + xScale.domain([0, 50]); + let offBarLabels = renderer.content().selectAll(".off-bar-label"); + assert.operator(offBarLabels.size(), ">", 0, "some off-bar labels are drawn"); + let offBarLabelVisibilities: string[] = []; + offBarLabels.each(function() { + offBarLabelVisibilities.push(d3.select(this).style("visibility")); + }); + assert.isTrue(offBarLabelVisibilities.every((visibility) => visibility === "hidden"), "all off-bar labels are hidden"); + svg.remove(); + }); }); describe("Stacked Bar Plot Weird Values", () => { From 440c812091486394154580089c388a153d2a56ce Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Tue, 22 Sep 2015 16:09:55 -0700 Subject: [PATCH 40/49] Use a CSS rule to hide off-bar labels on StackedBar. Normally we shouldn't use !important, but it seemed a cleaner solution compared to adding new logic to hide the labels on StackedBar. The rule has a HACKHACK that points to #2795. --- plottable.css | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/plottable.css b/plottable.css index 66cf08fe6d..e6a2eaecd1 100644 --- a/plottable.css +++ b/plottable.css @@ -83,6 +83,11 @@ svg.plottable { fill: #32313F; } +.plottable .stacked-bar-plot .off-bar-label { + /* HACKHACK #2795: correct off-bar label logic to be implemented on StackedBar */ + visibility: hidden !important; +} + .plottable .axis-label text { font-size: 10px; font-weight: bold; @@ -215,4 +220,4 @@ svg.plottable { .plottable .pie-plot .arc.outline { stroke-linejoin: round; -} \ No newline at end of file +} From dde58cac014039560276ff444a5e97dc97579621 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Tue, 22 Sep 2015 16:20:40 -0700 Subject: [PATCH 41/49] [Plots.Bar] Clean up tests and implementation a bit. --- plottable.js | 2 +- src/plots/barPlot.ts | 2 +- test/plots/barPlotTests.ts | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/plottable.js b/plottable.js index 96a9c01085..893bda37fe 100644 --- a/plottable.js +++ b/plottable.js @@ -8708,7 +8708,7 @@ var Plottable; var scaledBaseline = valueScale != null ? valueScale.scale(_this.baselineValue()) : _this.baselineValue(); var barWidth = attrToProjector["width"](d, i, dataset); var barHeight = attrToProjector["height"](d, i, dataset); - var text = _this._labelFormatter(valueAccessor(d, i, dataset)).toString(); + var text = _this._labelFormatter(valueAccessor(d, i, dataset)); var measurement = measurer.measure(text); var xAlignment = "center"; var yAlignment = "center"; diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index e66d15051e..e8021367cc 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -478,7 +478,7 @@ export module Plots { let barWidth = attrToProjector["width"](d, i, dataset); let barHeight = attrToProjector["height"](d, i, dataset); - let text = this._labelFormatter(valueAccessor(d, i, dataset)).toString(); + let text = this._labelFormatter(valueAccessor(d, i, dataset)); let measurement = measurer.measure(text); let xAlignment = "center"; diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 097cf5e890..606f01618a 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -962,7 +962,7 @@ describe("Plots", () => { it("formatters are used properly", () => { barPlot.labelsEnabled(true); - let formatter = (n: number) => n.toString() + "%"; + let formatter = (n: number) => `${n}%`; barPlot.labelFormatter(formatter); barPlot.renderTo(svg); let texts = barPlot.content().selectAll("text"); @@ -974,7 +974,7 @@ describe("Plots", () => { svg.remove(); }); - it("bar labels are shown inside or outside the bar as appropriate", () => { + it("shows labels inside or outside the bar as appropriate", () => { barPlot.labelsEnabled(true); barPlot.renderTo(svg); From f529da4ae6185a18df69852b1d377469f7f32435 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Tue, 22 Sep 2015 16:52:55 -0700 Subject: [PATCH 42/49] [Plots.Bar] Made the on-bar/off-bar label test less fragile. --- test/plots/barPlotTests.ts | 42 ++++++++++++++++++++++++-------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 606f01618a..fb3b87a2e9 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -764,17 +764,23 @@ describe("Plots", () => { barPlot.renderTo(svg); }); - it("shows both inner and outer labels", () => { + it("shows labels inside or outside the bar as appropriate", () => { barPlot.renderTo(svg); - let texts = barPlot.content().selectAll("text"); - assert.strictEqual(texts.size(), dataset.data().length, "one label drawn per datum"); - - let offBarLabels = barPlot.content().selectAll(".off-bar-label"); - assert.strictEqual(offBarLabels.size(), 3, "there are 3 off-bar labels"); + let labels = barPlot.content().selectAll(".on-bar-label, .off-bar-label"); + assert.strictEqual(labels.size(), dataset.data().length, "one label drawn per datum"); + + let bars = barPlot.content().select(".bar-area").selectAll("rect"); + labels.each((d, i) => { + let labelBoundingClientRect = ( labels[0][i]).getBoundingClientRect(); + let barBoundingClientRect = ( bars[0][i]).getBoundingClientRect(); + if (labelBoundingClientRect.width > barBoundingClientRect.width) { + assert.isTrue(d3.select(labels[0][i]).classed("off-bar-label"), `label with index ${i} doesn't fit and carries the off-bar class`); + } else { + assert.isTrue(d3.select(labels[0][i]).classed("on-bar-label"), `label with index ${i} fits and carries the on-bar class`); + } + }); - let onBarLabels = barPlot.content().selectAll(".on-bar-label"); - assert.strictEqual(onBarLabels.size(), 2, "there are 2 on-bar labels"); svg.remove(); }); @@ -978,14 +984,20 @@ describe("Plots", () => { barPlot.labelsEnabled(true); barPlot.renderTo(svg); - let texts = barPlot.content().selectAll("text"); - assert.strictEqual(texts.size(), dataset.data().length, "one label drawn per datum"); - - let offBarLabels = barPlot.content().selectAll(".off-bar-label"); - assert.strictEqual(offBarLabels.size(), 3, "there are 3 off-bar labels"); + let labels = barPlot.content().selectAll(".on-bar-label, .off-bar-label"); + assert.strictEqual(labels.size(), dataset.data().length, "one label drawn per datum"); + + let bars = barPlot.content().select(".bar-area").selectAll("rect"); + labels.each((d, i) => { + let labelBoundingClientRect = ( labels[0][i]).getBoundingClientRect(); + let barBoundingClientRect = ( bars[0][i]).getBoundingClientRect(); + if (labelBoundingClientRect.height > barBoundingClientRect.height) { + assert.isTrue(d3.select(labels[0][i]).classed("off-bar-label"), `label with index ${i} doesn't fit and carries the off-bar class`); + } else { + assert.isTrue(d3.select(labels[0][i]).classed("on-bar-label"), `label with index ${i} fits and carries the on-bar class`); + } + }); - let onBarLabels = barPlot.content().selectAll(".on-bar-label"); - assert.strictEqual(onBarLabels.size(), 2, "there are 2 on-bar labels"); svg.remove(); }); From e4a000728f58353e7514d5f3bcb66b6cae1aa993 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Tue, 22 Sep 2015 17:01:21 -0700 Subject: [PATCH 43/49] tslint (line length) --- test/plots/barPlotTests.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index fb3b87a2e9..64434532fc 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -775,9 +775,11 @@ describe("Plots", () => { let labelBoundingClientRect = ( labels[0][i]).getBoundingClientRect(); let barBoundingClientRect = ( bars[0][i]).getBoundingClientRect(); if (labelBoundingClientRect.width > barBoundingClientRect.width) { - assert.isTrue(d3.select(labels[0][i]).classed("off-bar-label"), `label with index ${i} doesn't fit and carries the off-bar class`); + assert.isTrue(d3.select(labels[0][i]).classed("off-bar-label"), + `label with index ${i} doesn't fit and carries the off-bar class`); } else { - assert.isTrue(d3.select(labels[0][i]).classed("on-bar-label"), `label with index ${i} fits and carries the on-bar class`); + assert.isTrue(d3.select(labels[0][i]).classed("on-bar-label"), + `label with index ${i} fits and carries the on-bar class`); } }); @@ -992,9 +994,11 @@ describe("Plots", () => { let labelBoundingClientRect = ( labels[0][i]).getBoundingClientRect(); let barBoundingClientRect = ( bars[0][i]).getBoundingClientRect(); if (labelBoundingClientRect.height > barBoundingClientRect.height) { - assert.isTrue(d3.select(labels[0][i]).classed("off-bar-label"), `label with index ${i} doesn't fit and carries the off-bar class`); + assert.isTrue(d3.select(labels[0][i]).classed("off-bar-label"), + `label with index ${i} doesn't fit and carries the off-bar class`); } else { - assert.isTrue(d3.select(labels[0][i]).classed("on-bar-label"), `label with index ${i} fits and carries the on-bar class`); + assert.isTrue(d3.select(labels[0][i]).classed("on-bar-label"), + `label with index ${i} fits and carries the on-bar class`); } }); From 3356fff8090d552ae4389824b51bfc47624a99b0 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Wed, 23 Sep 2015 11:44:22 -0700 Subject: [PATCH 44/49] [Plots.Bar] Refactor label logic to use assignments, not +=/-= --- plottable.js | 42 ++++++++++++++++++++++-------------------- src/plots/barPlot.ts | 42 ++++++++++++++++++++++-------------------- 2 files changed, 44 insertions(+), 40 deletions(-) diff --git a/plottable.js b/plottable.js index 893bda37fe..651e281819 100644 --- a/plottable.js +++ b/plottable.js @@ -8712,72 +8712,74 @@ var Plottable; var measurement = measurer.measure(text); var xAlignment = "center"; var yAlignment = "center"; + var barX = attrToProjector["x"](d, i, dataset); + var barY = attrToProjector["y"](d, i, dataset); var labelContainerOrigin = { - x: attrToProjector["x"](d, i, dataset), - y: attrToProjector["y"](d, i, dataset) + x: barX, + y: barY }; var containerWidth = barWidth; var containerHeight = barHeight; var labelOrigin = { - x: labelContainerOrigin.x, - y: labelContainerOrigin.y + x: barX, + y: barY }; var showLabelOnBar = _this._isVertical ? (measurement.height <= barHeight) : (measurement.width <= barWidth); if (_this._isVertical) { - labelOrigin.x += containerWidth / 2 - measurement.width / 2; + labelOrigin.x = barX + containerWidth / 2 - measurement.width / 2; if (showLabelOnBar) { var offset = Math.min((barHeight - measurement.height) / 2, Bar._LABEL_VERTICAL_PADDING); if (scaledValue < scaledBaseline) { - labelContainerOrigin.y += offset; + labelContainerOrigin.y = barY + offset; yAlignment = "top"; - labelOrigin.y += offset; + labelOrigin.y = barY + offset; } else { - labelContainerOrigin.y -= offset; + labelContainerOrigin.y = barY - offset; yAlignment = "bottom"; - labelOrigin.y += containerHeight - offset - measurement.height; + labelOrigin.y = barY + containerHeight - offset - measurement.height; } } else { var offset = Bar._LABEL_VERTICAL_PADDING; containerHeight = barHeight + offset + measurement.height; if (scaledValue <= scaledBaseline) { - labelContainerOrigin.y -= offset + measurement.height; + labelContainerOrigin.y = barY - (offset + measurement.height); yAlignment = "top"; - labelOrigin.y -= offset + measurement.height; + labelOrigin.y = barY - (offset + measurement.height); } else { yAlignment = "bottom"; - labelOrigin.y += barHeight + offset; + labelOrigin.y = barY + barHeight + offset; } } } else { - labelOrigin.y += containerHeight / 2 - measurement.height / 2; + labelOrigin.y = barY + containerHeight / 2 - measurement.height / 2; if (showLabelOnBar) { var offset = Math.min((barWidth - measurement.width) / 2, Bar._LABEL_HORIZONTAL_PADDING); if (scaledValue < scaledBaseline) { - labelContainerOrigin.x += offset; + labelContainerOrigin.x = barX + offset; xAlignment = "left"; - labelOrigin.x += offset; + labelOrigin.x = barX + offset; } else { - labelContainerOrigin.x -= offset; + labelContainerOrigin.x = barX - offset; xAlignment = "right"; - labelOrigin.x += containerWidth - offset - measurement.width; + labelOrigin.x = barX + containerWidth - offset - measurement.width; } } else { var offset = Bar._LABEL_HORIZONTAL_PADDING; containerWidth = barWidth + offset + measurement.width; if (scaledValue < scaledBaseline) { - labelContainerOrigin.x -= offset + measurement.width; + labelContainerOrigin.x = barX - (offset + measurement.width); xAlignment = "left"; - labelOrigin.x -= offset + measurement.width; + labelOrigin.x = barX - (offset + measurement.width); } else { xAlignment = "right"; - labelOrigin.x += barWidth + offset; + labelOrigin.x = barX + barWidth + offset; } } } diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index e8021367cc..cbc2829c67 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -483,70 +483,72 @@ export module Plots { let xAlignment = "center"; let yAlignment = "center"; + let barX = attrToProjector["x"](d, i, dataset); + let barY = attrToProjector["y"](d, i, dataset); let labelContainerOrigin = { - x: attrToProjector["x"](d, i, dataset), - y: attrToProjector["y"](d, i, dataset) + x: barX, + y: barY }; let containerWidth = barWidth; let containerHeight = barHeight; let labelOrigin = { - x: labelContainerOrigin.x, - y: labelContainerOrigin.y + x: barX, + y: barY }; let showLabelOnBar = this._isVertical ? (measurement.height <= barHeight) : (measurement.width <= barWidth); if (this._isVertical) { - labelOrigin.x += containerWidth / 2 - measurement.width / 2; + labelOrigin.x = barX + containerWidth / 2 - measurement.width / 2; if (showLabelOnBar) { let offset = Math.min( (barHeight - measurement.height ) / 2, Bar._LABEL_VERTICAL_PADDING); if (scaledValue < scaledBaseline) { - labelContainerOrigin.y += offset; + labelContainerOrigin.y = barY + offset; yAlignment = "top"; - labelOrigin.y += offset; + labelOrigin.y = barY + offset; } else { - labelContainerOrigin.y -= offset; + labelContainerOrigin.y = barY - offset; yAlignment = "bottom"; - labelOrigin.y += containerHeight - offset - measurement.height; + labelOrigin.y = barY + containerHeight - offset - measurement.height; } } else { // show label off bar let offset = Bar._LABEL_VERTICAL_PADDING; containerHeight = barHeight + offset + measurement.height; if (scaledValue <= scaledBaseline) { - labelContainerOrigin.y -= offset + measurement.height; + labelContainerOrigin.y = barY - (offset + measurement.height); yAlignment = "top"; - labelOrigin.y -= offset + measurement.height; + labelOrigin.y = barY - (offset + measurement.height); } else { yAlignment = "bottom"; - labelOrigin.y += barHeight + offset; + labelOrigin.y = barY + barHeight + offset; } } } else { // horizontal - labelOrigin.y += containerHeight / 2 - measurement.height / 2; + labelOrigin.y = barY + containerHeight / 2 - measurement.height / 2; if (showLabelOnBar) { let offset = Math.min( (barWidth - measurement.width ) / 2, Bar._LABEL_HORIZONTAL_PADDING); if (scaledValue < scaledBaseline) { - labelContainerOrigin.x += offset; + labelContainerOrigin.x = barX + offset; xAlignment = "left"; - labelOrigin.x += offset; + labelOrigin.x = barX + offset; } else { - labelContainerOrigin.x -= offset; + labelContainerOrigin.x = barX - offset; xAlignment = "right"; - labelOrigin.x += containerWidth - offset - measurement.width; + labelOrigin.x = barX + containerWidth - offset - measurement.width; } } else { // show label off bar let offset = Bar._LABEL_HORIZONTAL_PADDING; containerWidth = barWidth + offset + measurement.width; if (scaledValue < scaledBaseline) { - labelContainerOrigin.x -= offset + measurement.width; + labelContainerOrigin.x = barX - (offset + measurement.width); xAlignment = "left"; - labelOrigin.x -= offset + measurement.width; + labelOrigin.x = barX - (offset + measurement.width); } else { xAlignment = "right"; - labelOrigin.x += barWidth + offset; + labelOrigin.x = barX + barWidth + offset; } } } From 858da628d5304d5c9b19c4e16e5325b899c27db9 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Wed, 23 Sep 2015 13:10:48 -0700 Subject: [PATCH 45/49] WIP space-aware off-bar --- plottable.js | 6 ++++-- src/plots/barPlot.ts | 12 ++++++++++-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/plottable.js b/plottable.js index 651e281819..05240b3c90 100644 --- a/plottable.js +++ b/plottable.js @@ -8739,8 +8739,9 @@ var Plottable; yAlignment = "bottom"; labelOrigin.y = barY + containerHeight - offset - measurement.height; } + showLabelOnBar = showLabelOnBar && labelOrigin.y >= 0 && labelOrigin.y + measurement.height <= _this.height(); } - else { + if (!showLabelOnBar) { var offset = Bar._LABEL_VERTICAL_PADDING; containerHeight = barHeight + offset + measurement.height; if (scaledValue <= scaledBaseline) { @@ -8768,8 +8769,9 @@ var Plottable; xAlignment = "right"; labelOrigin.x = barX + containerWidth - offset - measurement.width; } + showLabelOnBar = showLabelOnBar && labelOrigin.x >= 0 && labelOrigin.x + measurement.width <= _this.width(); } - else { + if (!showLabelOnBar) { var offset = Bar._LABEL_HORIZONTAL_PADDING; containerWidth = barWidth + offset + measurement.width; if (scaledValue < scaledBaseline) { diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index cbc2829c67..3e2ff3102e 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -513,7 +513,11 @@ export module Plots { yAlignment = "bottom"; labelOrigin.y = barY + containerHeight - offset - measurement.height; } - } else { // show label off bar + + showLabelOnBar = showLabelOnBar && labelOrigin.y >= 0 && labelOrigin.y + measurement.height <= this.height(); + } + + if (!showLabelOnBar) { // show label off bar let offset = Bar._LABEL_VERTICAL_PADDING; containerHeight = barHeight + offset + measurement.height; if (scaledValue <= scaledBaseline) { @@ -539,7 +543,11 @@ export module Plots { xAlignment = "right"; labelOrigin.x = barX + containerWidth - offset - measurement.width; } - } else { // show label off bar + + showLabelOnBar = showLabelOnBar && labelOrigin.x >= 0 && labelOrigin.x + measurement.width <= this.width(); + } + + if (!showLabelOnBar) { // show label off bar let offset = Bar._LABEL_HORIZONTAL_PADDING; containerWidth = barWidth + offset + measurement.width; if (scaledValue < scaledBaseline) { From ad929c4b88e9aebd71be352818170c8e3abfd137 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Wed, 23 Sep 2015 16:28:42 -0700 Subject: [PATCH 46/49] Revert "WIP space-aware off-bar" Came up with a better way to do this feature. This reverts commit 858da628d5304d5c9b19c4e16e5325b899c27db9. --- plottable.js | 6 ++---- src/plots/barPlot.ts | 12 ++---------- 2 files changed, 4 insertions(+), 14 deletions(-) diff --git a/plottable.js b/plottable.js index 05240b3c90..651e281819 100644 --- a/plottable.js +++ b/plottable.js @@ -8739,9 +8739,8 @@ var Plottable; yAlignment = "bottom"; labelOrigin.y = barY + containerHeight - offset - measurement.height; } - showLabelOnBar = showLabelOnBar && labelOrigin.y >= 0 && labelOrigin.y + measurement.height <= _this.height(); } - if (!showLabelOnBar) { + else { var offset = Bar._LABEL_VERTICAL_PADDING; containerHeight = barHeight + offset + measurement.height; if (scaledValue <= scaledBaseline) { @@ -8769,9 +8768,8 @@ var Plottable; xAlignment = "right"; labelOrigin.x = barX + containerWidth - offset - measurement.width; } - showLabelOnBar = showLabelOnBar && labelOrigin.x >= 0 && labelOrigin.x + measurement.width <= _this.width(); } - if (!showLabelOnBar) { + else { var offset = Bar._LABEL_HORIZONTAL_PADDING; containerWidth = barWidth + offset + measurement.width; if (scaledValue < scaledBaseline) { diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 3e2ff3102e..cbc2829c67 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -513,11 +513,7 @@ export module Plots { yAlignment = "bottom"; labelOrigin.y = barY + containerHeight - offset - measurement.height; } - - showLabelOnBar = showLabelOnBar && labelOrigin.y >= 0 && labelOrigin.y + measurement.height <= this.height(); - } - - if (!showLabelOnBar) { // show label off bar + } else { // show label off bar let offset = Bar._LABEL_VERTICAL_PADDING; containerHeight = barHeight + offset + measurement.height; if (scaledValue <= scaledBaseline) { @@ -543,11 +539,7 @@ export module Plots { xAlignment = "right"; labelOrigin.x = barX + containerWidth - offset - measurement.width; } - - showLabelOnBar = showLabelOnBar && labelOrigin.x >= 0 && labelOrigin.x + measurement.width <= this.width(); - } - - if (!showLabelOnBar) { // show label off bar + } else { // show label off bar let offset = Bar._LABEL_HORIZONTAL_PADDING; containerWidth = barWidth + offset + measurement.width; if (scaledValue < scaledBaseline) { From 1f13ecf6c6aa0c56d56a405f415fce83090c35bb Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Wed, 23 Sep 2015 16:29:01 -0700 Subject: [PATCH 47/49] Revert "[Plots.Bar] Refactor label logic to use assignments, not +=/-=" Came up with a better way to do space-aware off-bar labels. This reverts commit 3356fff8090d552ae4389824b51bfc47624a99b0. --- plottable.js | 42 ++++++++++++++++++++---------------------- src/plots/barPlot.ts | 42 ++++++++++++++++++++---------------------- 2 files changed, 40 insertions(+), 44 deletions(-) diff --git a/plottable.js b/plottable.js index 651e281819..893bda37fe 100644 --- a/plottable.js +++ b/plottable.js @@ -8712,74 +8712,72 @@ var Plottable; var measurement = measurer.measure(text); var xAlignment = "center"; var yAlignment = "center"; - var barX = attrToProjector["x"](d, i, dataset); - var barY = attrToProjector["y"](d, i, dataset); var labelContainerOrigin = { - x: barX, - y: barY + x: attrToProjector["x"](d, i, dataset), + y: attrToProjector["y"](d, i, dataset) }; var containerWidth = barWidth; var containerHeight = barHeight; var labelOrigin = { - x: barX, - y: barY + x: labelContainerOrigin.x, + y: labelContainerOrigin.y }; var showLabelOnBar = _this._isVertical ? (measurement.height <= barHeight) : (measurement.width <= barWidth); if (_this._isVertical) { - labelOrigin.x = barX + containerWidth / 2 - measurement.width / 2; + labelOrigin.x += containerWidth / 2 - measurement.width / 2; if (showLabelOnBar) { var offset = Math.min((barHeight - measurement.height) / 2, Bar._LABEL_VERTICAL_PADDING); if (scaledValue < scaledBaseline) { - labelContainerOrigin.y = barY + offset; + labelContainerOrigin.y += offset; yAlignment = "top"; - labelOrigin.y = barY + offset; + labelOrigin.y += offset; } else { - labelContainerOrigin.y = barY - offset; + labelContainerOrigin.y -= offset; yAlignment = "bottom"; - labelOrigin.y = barY + containerHeight - offset - measurement.height; + labelOrigin.y += containerHeight - offset - measurement.height; } } else { var offset = Bar._LABEL_VERTICAL_PADDING; containerHeight = barHeight + offset + measurement.height; if (scaledValue <= scaledBaseline) { - labelContainerOrigin.y = barY - (offset + measurement.height); + labelContainerOrigin.y -= offset + measurement.height; yAlignment = "top"; - labelOrigin.y = barY - (offset + measurement.height); + labelOrigin.y -= offset + measurement.height; } else { yAlignment = "bottom"; - labelOrigin.y = barY + barHeight + offset; + labelOrigin.y += barHeight + offset; } } } else { - labelOrigin.y = barY + containerHeight / 2 - measurement.height / 2; + labelOrigin.y += containerHeight / 2 - measurement.height / 2; if (showLabelOnBar) { var offset = Math.min((barWidth - measurement.width) / 2, Bar._LABEL_HORIZONTAL_PADDING); if (scaledValue < scaledBaseline) { - labelContainerOrigin.x = barX + offset; + labelContainerOrigin.x += offset; xAlignment = "left"; - labelOrigin.x = barX + offset; + labelOrigin.x += offset; } else { - labelContainerOrigin.x = barX - offset; + labelContainerOrigin.x -= offset; xAlignment = "right"; - labelOrigin.x = barX + containerWidth - offset - measurement.width; + labelOrigin.x += containerWidth - offset - measurement.width; } } else { var offset = Bar._LABEL_HORIZONTAL_PADDING; containerWidth = barWidth + offset + measurement.width; if (scaledValue < scaledBaseline) { - labelContainerOrigin.x = barX - (offset + measurement.width); + labelContainerOrigin.x -= offset + measurement.width; xAlignment = "left"; - labelOrigin.x = barX - (offset + measurement.width); + labelOrigin.x -= offset + measurement.width; } else { xAlignment = "right"; - labelOrigin.x = barX + barWidth + offset; + labelOrigin.x += barWidth + offset; } } } diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index cbc2829c67..e8021367cc 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -483,72 +483,70 @@ export module Plots { let xAlignment = "center"; let yAlignment = "center"; - let barX = attrToProjector["x"](d, i, dataset); - let barY = attrToProjector["y"](d, i, dataset); let labelContainerOrigin = { - x: barX, - y: barY + x: attrToProjector["x"](d, i, dataset), + y: attrToProjector["y"](d, i, dataset) }; let containerWidth = barWidth; let containerHeight = barHeight; let labelOrigin = { - x: barX, - y: barY + x: labelContainerOrigin.x, + y: labelContainerOrigin.y }; let showLabelOnBar = this._isVertical ? (measurement.height <= barHeight) : (measurement.width <= barWidth); if (this._isVertical) { - labelOrigin.x = barX + containerWidth / 2 - measurement.width / 2; + labelOrigin.x += containerWidth / 2 - measurement.width / 2; if (showLabelOnBar) { let offset = Math.min( (barHeight - measurement.height ) / 2, Bar._LABEL_VERTICAL_PADDING); if (scaledValue < scaledBaseline) { - labelContainerOrigin.y = barY + offset; + labelContainerOrigin.y += offset; yAlignment = "top"; - labelOrigin.y = barY + offset; + labelOrigin.y += offset; } else { - labelContainerOrigin.y = barY - offset; + labelContainerOrigin.y -= offset; yAlignment = "bottom"; - labelOrigin.y = barY + containerHeight - offset - measurement.height; + labelOrigin.y += containerHeight - offset - measurement.height; } } else { // show label off bar let offset = Bar._LABEL_VERTICAL_PADDING; containerHeight = barHeight + offset + measurement.height; if (scaledValue <= scaledBaseline) { - labelContainerOrigin.y = barY - (offset + measurement.height); + labelContainerOrigin.y -= offset + measurement.height; yAlignment = "top"; - labelOrigin.y = barY - (offset + measurement.height); + labelOrigin.y -= offset + measurement.height; } else { yAlignment = "bottom"; - labelOrigin.y = barY + barHeight + offset; + labelOrigin.y += barHeight + offset; } } } else { // horizontal - labelOrigin.y = barY + containerHeight / 2 - measurement.height / 2; + labelOrigin.y += containerHeight / 2 - measurement.height / 2; if (showLabelOnBar) { let offset = Math.min( (barWidth - measurement.width ) / 2, Bar._LABEL_HORIZONTAL_PADDING); if (scaledValue < scaledBaseline) { - labelContainerOrigin.x = barX + offset; + labelContainerOrigin.x += offset; xAlignment = "left"; - labelOrigin.x = barX + offset; + labelOrigin.x += offset; } else { - labelContainerOrigin.x = barX - offset; + labelContainerOrigin.x -= offset; xAlignment = "right"; - labelOrigin.x = barX + containerWidth - offset - measurement.width; + labelOrigin.x += containerWidth - offset - measurement.width; } } else { // show label off bar let offset = Bar._LABEL_HORIZONTAL_PADDING; containerWidth = barWidth + offset + measurement.width; if (scaledValue < scaledBaseline) { - labelContainerOrigin.x = barX - (offset + measurement.width); + labelContainerOrigin.x -= offset + measurement.width; xAlignment = "left"; - labelOrigin.x = barX - (offset + measurement.width); + labelOrigin.x -= offset + measurement.width; } else { xAlignment = "right"; - labelOrigin.x = barX + barWidth + offset; + labelOrigin.x += barWidth + offset; } } } From 2e0e5fdf2cdec951a743849829a6d1bd0c8de579 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Wed, 23 Sep 2015 16:16:03 -0700 Subject: [PATCH 48/49] [Plots.Bar] Move label off-bar if it doesn't fit in the part of the bar visible on screen --- plottable.js | 22 ++++++++--- src/plots/barPlot.ts | 24 +++++++++--- test/plots/barPlotTests.ts | 80 ++++++++++++++++++++++++++------------ 3 files changed, 91 insertions(+), 35 deletions(-) diff --git a/plottable.js b/plottable.js index 893bda37fe..71e492e0e6 100644 --- a/plottable.js +++ b/plottable.js @@ -8722,11 +8722,18 @@ var Plottable; x: labelContainerOrigin.x, y: labelContainerOrigin.y }; - var showLabelOnBar = _this._isVertical ? (measurement.height <= barHeight) : (measurement.width <= barWidth); + var showLabelOnBar = true; if (_this._isVertical) { labelOrigin.x += containerWidth / 2 - measurement.width / 2; + var barY = attrToProjector["y"](d, i, dataset); + var effectiveBarHeight = Plottable.Utils.Math.min([ + _this.height() - barY, + barY + barHeight, + barHeight + ], 0); + var offset = Bar._LABEL_VERTICAL_PADDING; + showLabelOnBar = measurement.height + 2 * offset <= effectiveBarHeight; if (showLabelOnBar) { - var offset = Math.min((barHeight - measurement.height) / 2, Bar._LABEL_VERTICAL_PADDING); if (scaledValue < scaledBaseline) { labelContainerOrigin.y += offset; yAlignment = "top"; @@ -8739,7 +8746,6 @@ var Plottable; } } else { - var offset = Bar._LABEL_VERTICAL_PADDING; containerHeight = barHeight + offset + measurement.height; if (scaledValue <= scaledBaseline) { labelContainerOrigin.y -= offset + measurement.height; @@ -8754,8 +8760,15 @@ var Plottable; } else { labelOrigin.y += containerHeight / 2 - measurement.height / 2; + var barX = attrToProjector["x"](d, i, dataset); + var effectiveBarWidth = Plottable.Utils.Math.min([ + _this.width() - barX, + barX + barWidth, + barWidth + ], 0); + var offset = Bar._LABEL_HORIZONTAL_PADDING; + showLabelOnBar = measurement.width + 2 * offset <= effectiveBarWidth; if (showLabelOnBar) { - var offset = Math.min((barWidth - measurement.width) / 2, Bar._LABEL_HORIZONTAL_PADDING); if (scaledValue < scaledBaseline) { labelContainerOrigin.x += offset; xAlignment = "left"; @@ -8768,7 +8781,6 @@ var Plottable; } } else { - var offset = Bar._LABEL_HORIZONTAL_PADDING; containerWidth = barWidth + offset + measurement.width; if (scaledValue < scaledBaseline) { labelContainerOrigin.x -= offset + measurement.width; diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index e8021367cc..167d844fe5 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -495,13 +495,21 @@ export module Plots { y: labelContainerOrigin.y }; - let showLabelOnBar = this._isVertical ? (measurement.height <= barHeight) : (measurement.width <= barWidth); + let showLabelOnBar = true; if (this._isVertical) { labelOrigin.x += containerWidth / 2 - measurement.width / 2; + let barY = attrToProjector["y"](d, i, dataset); + let effectiveBarHeight = Utils.Math.min([ + this.height() - barY, + barY + barHeight, + barHeight + ], 0); + let offset = Bar._LABEL_VERTICAL_PADDING; + showLabelOnBar = measurement.height + 2 * offset <= effectiveBarHeight; + if (showLabelOnBar) { - let offset = Math.min( (barHeight - measurement.height ) / 2, Bar._LABEL_VERTICAL_PADDING); if (scaledValue < scaledBaseline) { labelContainerOrigin.y += offset; yAlignment = "top"; @@ -512,7 +520,6 @@ export module Plots { labelOrigin.y += containerHeight - offset - measurement.height; } } else { // show label off bar - let offset = Bar._LABEL_VERTICAL_PADDING; containerHeight = barHeight + offset + measurement.height; if (scaledValue <= scaledBaseline) { labelContainerOrigin.y -= offset + measurement.height; @@ -526,8 +533,16 @@ export module Plots { } else { // horizontal labelOrigin.y += containerHeight / 2 - measurement.height / 2; + let barX = attrToProjector["x"](d, i, dataset); + let effectiveBarWidth = Utils.Math.min([ + this.width() - barX, + barX + barWidth, + barWidth + ], 0); + let offset = Bar._LABEL_HORIZONTAL_PADDING; + showLabelOnBar = measurement.width + 2 * offset <= effectiveBarWidth; + if (showLabelOnBar) { - let offset = Math.min( (barWidth - measurement.width ) / 2, Bar._LABEL_HORIZONTAL_PADDING); if (scaledValue < scaledBaseline) { labelContainerOrigin.x += offset; xAlignment = "left"; @@ -538,7 +553,6 @@ export module Plots { labelOrigin.x += containerWidth - offset - measurement.width; } } else { // show label off bar - let offset = Bar._LABEL_HORIZONTAL_PADDING; containerWidth = barWidth + offset + measurement.width; if (scaledValue < scaledBaseline) { labelContainerOrigin.x -= offset + measurement.width; diff --git a/test/plots/barPlotTests.ts b/test/plots/barPlotTests.ts index 64434532fc..cbaaa866f1 100644 --- a/test/plots/barPlotTests.ts +++ b/test/plots/barPlotTests.ts @@ -787,27 +787,43 @@ describe("Plots", () => { }); it("hides labels cut off by the right edge", () => { + barPlot.labelsEnabled(true); + let labels = barPlot.content().selectAll(".on-bar-label, .off-bar-label"); + let centerXValues = labels.select("text")[0].map((textNode) => xScale.invert(getCenterOfText( textNode).x)); + let wasOriginallyOnBar = labels[0].map((label) => d3.select(label).classed("on-bar-label")); + dataset.data().forEach((d, i) => { - let texts = svg.selectAll("text"); - let centerOfText = getCenterOfText( texts[0][i]); - let centerXValue = xScale.invert(centerOfText.x); + let centerXValue = centerXValues[i]; xScale.domain([centerXValue - (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0]), centerXValue]); - - texts = svg.selectAll("text"); // re-select after rendering - assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + labels = barPlot.content().selectAll(".on-bar-label, .off-bar-label"); // re-select after rendering + if (wasOriginallyOnBar[i] && d.x < 0) { + assert.isTrue(d3.select(labels[0][i]).classed("off-bar-label"), + `cut off on-bar label was switched to off-bar (index ${i})`); + } else { + let textNode = labels.select("text")[0][i]; + assert.strictEqual(d3.select(textNode).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + } }); svg.remove(); }); it("hides labels cut off by the left edge", () => { + barPlot.labelsEnabled(true); + let labels = barPlot.content().selectAll(".on-bar-label, .off-bar-label"); + let centerXValues = labels.select("text")[0].map((textNode) => xScale.invert(getCenterOfText( textNode).x)); + let wasOriginallyOnBar = labels[0].map((label) => d3.select(label).classed("on-bar-label")); + dataset.data().forEach((d, i) => { - let texts = svg.selectAll("text"); - let centerOfText = getCenterOfText( texts[0][i]); - let centerXValue = xScale.invert(centerOfText.x); + let centerXValue = centerXValues[i]; xScale.domain([centerXValue, centerXValue + (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0])]); - - texts = svg.selectAll("text"); // re-select after rendering - assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + labels = barPlot.content().selectAll(".on-bar-label, .off-bar-label"); // re-select after rendering + if (wasOriginallyOnBar[i] && d.x > 0) { + assert.isTrue(d3.select(labels[0][i]).classed("off-bar-label"), + `cut off on-bar label was switched to off-bar (index ${i})`); + } else { + let textNode = labels.select("text")[0][i]; + assert.strictEqual(d3.select(textNode).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + } }); svg.remove(); }); @@ -1069,30 +1085,44 @@ describe("Plots", () => { svg.remove(); }); - it("hides labels cut off by the top edge", () => { + it("hides or shifts labels cut off by the top edge", () => { barPlot.labelsEnabled(true); + let labels = barPlot.content().selectAll(".on-bar-label, .off-bar-label"); + let centerYValues = labels.select("text")[0].map((textNode) => yScale.invert(getCenterOfText( textNode).y)); + let wasOriginallyOnBar = labels[0].map((label) => d3.select(label).classed("on-bar-label")); + dataset.data().forEach((d, i) => { - let texts = svg.selectAll("text"); - let centerOfText = getCenterOfText( texts[0][i]); - let centerYValue = yScale.invert(centerOfText.y); + let centerYValue = centerYValues[i]; yScale.domain([centerYValue - (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0]), centerYValue]); - - texts = svg.selectAll("text"); // re-select after rendering - assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + labels = barPlot.content().selectAll(".on-bar-label, .off-bar-label"); // re-select after rendering + if (wasOriginallyOnBar[i] && d.y < 0) { + assert.isTrue(d3.select(labels[0][i]).classed("off-bar-label"), + `cut off on-bar label was switched to off-bar (index ${i})`); + } else { + let textNode = labels.select("text")[0][i]; + assert.strictEqual(d3.select(textNode).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + } }); svg.remove(); }); it("hides labels cut off by the bottom edge", () => { barPlot.labelsEnabled(true); + let labels = barPlot.content().selectAll(".on-bar-label, .off-bar-label"); + let centerYValues = labels.select("text")[0].map((textNode) => yScale.invert(getCenterOfText( textNode).y)); + let wasOriginallyOnBar = labels[0].map((label) => d3.select(label).classed("on-bar-label")); + dataset.data().forEach((d, i) => { - let texts = svg.selectAll("text"); - let centerOfText = getCenterOfText( texts[0][i]); - let centerYValue = yScale.invert(centerOfText.y); + let centerYValue = centerYValues[i]; yScale.domain([centerYValue, centerYValue + (DEFAULT_DOMAIN[1] - DEFAULT_DOMAIN[0])]); - - texts = svg.selectAll("text"); // re-select after rendering - assert.strictEqual(d3.select(texts[0][i]).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + labels = barPlot.content().selectAll(".on-bar-label, .off-bar-label"); // re-select after rendering + if (wasOriginallyOnBar[i] && d.y > 0) { + assert.isTrue(d3.select(labels[0][i]).classed("off-bar-label"), + `cut off on-bar label was switched to off-bar (index ${i})`); + } else { + let textNode = labels.select("text")[0][i]; + assert.strictEqual(d3.select(textNode).style("visibility"), "hidden", `label for bar with index ${i} is hidden`); + } }); svg.remove(); }); From d7b6319154dda89be8dc126ca9fc06882f70e372 Mon Sep 17 00:00:00 2001 From: Justin Lan Date: Wed, 23 Sep 2015 17:12:28 -0700 Subject: [PATCH 49/49] [Plots.Bar] Clarify space-aware off-bar logic. Switched to if-block instead of Math.min() --- plottable.js | 26 +++++++++++++++----------- src/plots/barPlot.ts | 24 +++++++++++++----------- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/plottable.js b/plottable.js index 71e492e0e6..c3bf7a60d5 100644 --- a/plottable.js +++ b/plottable.js @@ -8722,15 +8722,17 @@ var Plottable; x: labelContainerOrigin.x, y: labelContainerOrigin.y }; - var showLabelOnBar = true; + var showLabelOnBar; if (_this._isVertical) { labelOrigin.x += containerWidth / 2 - measurement.width / 2; var barY = attrToProjector["y"](d, i, dataset); - var effectiveBarHeight = Plottable.Utils.Math.min([ - _this.height() - barY, - barY + barHeight, - barHeight - ], 0); + var effectiveBarHeight = barHeight; + if (barY + barHeight > _this.height()) { + effectiveBarHeight = _this.height() - barY; + } + else if (barY < 0) { + effectiveBarHeight = barY + barHeight; + } var offset = Bar._LABEL_VERTICAL_PADDING; showLabelOnBar = measurement.height + 2 * offset <= effectiveBarHeight; if (showLabelOnBar) { @@ -8761,11 +8763,13 @@ var Plottable; else { labelOrigin.y += containerHeight / 2 - measurement.height / 2; var barX = attrToProjector["x"](d, i, dataset); - var effectiveBarWidth = Plottable.Utils.Math.min([ - _this.width() - barX, - barX + barWidth, - barWidth - ], 0); + var effectiveBarWidth = barWidth; + if (barX + barWidth > _this.width()) { + effectiveBarWidth = _this.width() - barX; + } + else if (barX < 0) { + effectiveBarWidth = barX + barWidth; + } var offset = Bar._LABEL_HORIZONTAL_PADDING; showLabelOnBar = measurement.width + 2 * offset <= effectiveBarWidth; if (showLabelOnBar) { diff --git a/src/plots/barPlot.ts b/src/plots/barPlot.ts index 167d844fe5..1331c10adc 100644 --- a/src/plots/barPlot.ts +++ b/src/plots/barPlot.ts @@ -495,17 +495,18 @@ export module Plots { y: labelContainerOrigin.y }; - let showLabelOnBar = true; + let showLabelOnBar: boolean; if (this._isVertical) { labelOrigin.x += containerWidth / 2 - measurement.width / 2; let barY = attrToProjector["y"](d, i, dataset); - let effectiveBarHeight = Utils.Math.min([ - this.height() - barY, - barY + barHeight, - barHeight - ], 0); + let effectiveBarHeight = barHeight; + if (barY + barHeight > this.height()) { + effectiveBarHeight = this.height() - barY; + } else if (barY < 0) { + effectiveBarHeight = barY + barHeight; + } let offset = Bar._LABEL_VERTICAL_PADDING; showLabelOnBar = measurement.height + 2 * offset <= effectiveBarHeight; @@ -534,11 +535,12 @@ export module Plots { labelOrigin.y += containerHeight / 2 - measurement.height / 2; let barX = attrToProjector["x"](d, i, dataset); - let effectiveBarWidth = Utils.Math.min([ - this.width() - barX, - barX + barWidth, - barWidth - ], 0); + let effectiveBarWidth = barWidth; + if (barX + barWidth > this.width()) { + effectiveBarWidth = this.width() - barX; + } else if (barX < 0) { + effectiveBarWidth = barX + barWidth; + } let offset = Bar._LABEL_HORIZONTAL_PADDING; showLabelOnBar = measurement.width + 2 * offset <= effectiveBarWidth;