From 0e8d53e70f3be6f937dc2697a2ef1e08da45b05a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 14 Jun 2019 16:02:52 -0400 Subject: [PATCH 1/4] lint: rename ax -> axKey ... to not confuse 'xa' & 'ya' with ax objects --- src/components/fx/hover.js | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/components/fx/hover.js b/src/components/fx/hover.js index a32ad3f0922..5ab07ccb2ac 100644 --- a/src/components/fx/hover.js +++ b/src/components/fx/hover.js @@ -839,7 +839,6 @@ function createHoverText(hoverData, opts, gd) { // show all the individual labels - // first create the objects var hoverLabels = container.selectAll('g.hovertext') .data(hoverData, function(d) { @@ -1055,7 +1054,7 @@ function createHoverText(hoverData, opts, gd) { // know what happens if the group spans all the way from one edge to // the other, though it hardly matters - there's just too much // information then. -function hoverAvoidOverlaps(hoverLabels, ax, fullLayout) { +function hoverAvoidOverlaps(hoverLabels, axKey, fullLayout) { var nummoves = 0; var axSign = 1; var nLabels = hoverLabels.size(); @@ -1064,10 +1063,13 @@ function hoverAvoidOverlaps(hoverLabels, ax, fullLayout) { var pointgroups = new Array(nLabels); hoverLabels.each(function(d, i) { - var axis = d[ax]; - var axIsX = axis._id.charAt(0) === 'x'; - var rng = axis.range; - if(!i && rng && ((rng[0] > rng[1]) !== axIsX)) axSign = -1; + var ax = d[axKey]; + var axIsX = ax._id.charAt(0) === 'x'; + var rng = ax.range; + + if(!i && rng && ((rng[0] > rng[1]) !== axIsX)) { + axSign = -1; + } pointgroups[i] = [{ datum: d, i: i, From a4b771c6cbb4f17319e35dcaca78d6a71bd9aed1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 14 Jun 2019 16:08:29 -0400 Subject: [PATCH 2/4] fix #3962 - don't build pointgroup array using d3-data indices - N.B. in the case where multiple item in a given d3-data bound array have identical key-function results, *only the first* of those items is considered. This implies that the `i` in e.g `selection.each(function(d, i) { })` does not always increment with a constant step. --- src/components/fx/hover.js | 10 ++++++---- test/jasmine/tests/box_test.js | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/components/fx/hover.js b/src/components/fx/hover.js index 5ab07ccb2ac..418856f1bef 100644 --- a/src/components/fx/hover.js +++ b/src/components/fx/hover.js @@ -842,6 +842,8 @@ function createHoverText(hoverData, opts, gd) { // first create the objects var hoverLabels = container.selectAll('g.hovertext') .data(hoverData, function(d) { + // N.B. when multiple items have the same result key-function value, + // only the first of those items in hoverData gets rendered return [d.trace.index, d.index, d.x0, d.y0, d.name, d.attr, d.xa, d.ya || ''].join(','); }); hoverLabels.enter().append('g') @@ -1061,18 +1063,18 @@ function hoverAvoidOverlaps(hoverLabels, axKey, fullLayout) { // make groups of touching points var pointgroups = new Array(nLabels); + var k = 0; - hoverLabels.each(function(d, i) { + hoverLabels.each(function(d) { var ax = d[axKey]; var axIsX = ax._id.charAt(0) === 'x'; var rng = ax.range; - if(!i && rng && ((rng[0] > rng[1]) !== axIsX)) { + if(k === 0 && rng && ((rng[0] > rng[1]) !== axIsX)) { axSign = -1; } - pointgroups[i] = [{ + pointgroups[k++] = [{ datum: d, - i: i, traceIndex: d.trace.index, dp: 0, pos: d.pos, diff --git a/test/jasmine/tests/box_test.js b/test/jasmine/tests/box_test.js index ba5321104b0..345898cef44 100644 --- a/test/jasmine/tests/box_test.js +++ b/test/jasmine/tests/box_test.js @@ -437,6 +437,25 @@ describe('Test box hover:', function() { }, nums: '0.6', name: 'pt #0' + }, { + desc: 'when zoomed in, cropping out labels', + mock: { + data: [{ + type: 'box', + y: [1, 2, 2, 3] + }], + layout: { + // cropping out q1 and max w/o failing during hoverAvoidOverlap + // https://github.com/plotly/plotly.js/issues/3962 + yaxis: {range: [1.6, 2.4]}, + width: 400, + height: 400 + } + }, + pos: [200, 200], + nums: ['median: 2', 'min: 1', 'q3: 2.5'], + name: ['', '', ''], + axis: 'trace 0' }].forEach(function(specs) { it('should generate correct hover labels ' + specs.desc, function(done) { run(specs).catch(failTest).then(done); From 1634ec80148d8c5d26933dd48a966ff21b298473 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 14 Jun 2019 16:32:43 -0400 Subject: [PATCH 3/4] make q1 hover label have higher "priority" than min ... so that if the min and q1 overlap and have the same overlap key-function output, q1 will be rendered, not min --- src/traces/box/hover.js | 2 +- test/jasmine/tests/box_test.js | 6 +++--- test/jasmine/tests/violin_test.js | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/traces/box/hover.js b/src/traces/box/hover.js index 0d47ca02855..06936675f29 100644 --- a/src/traces/box/hover.js +++ b/src/traces/box/hover.js @@ -151,7 +151,7 @@ function hoverOnBoxes(pointData, xval, yval, hovermode) { // box plots: each "point" gets many labels var usedVals = {}; - var attrs = ['med', 'min', 'q1', 'q3', 'max']; + var attrs = ['med', 'q1', 'q3', 'min', 'max']; if(trace.boxmean || (trace.meanline || {}).visible) { attrs.push('mean'); diff --git a/test/jasmine/tests/box_test.js b/test/jasmine/tests/box_test.js index 345898cef44..ef291630683 100644 --- a/test/jasmine/tests/box_test.js +++ b/test/jasmine/tests/box_test.js @@ -274,8 +274,8 @@ describe('Test box hover:', function() { return fig; }, nums: [ - 'q1: 0.3', 'median: 0.45', 'q3: 0.6', 'max: 1', 'median: 0.55', 'min: 0', 'min: 0.2', - 'q3: 0.6', 'max: 0.7', 'median: 0.45', 'min: 0.1', 'q3: 0.6', 'max: 0.9' + 'q1: 0.3', 'median: 0.45', 'q3: 0.6', 'max: 1', 'median: 0.55', 'min: 0', 'q1: 0.1', + 'q3: 0.6', 'max: 0.7', 'median: 0.45', 'q1: 0.2', 'q3: 0.6', 'max: 0.9' ], name: [ '', 'kale', '', '', 'radishes', '', '', @@ -453,7 +453,7 @@ describe('Test box hover:', function() { } }, pos: [200, 200], - nums: ['median: 2', 'min: 1', 'q3: 2.5'], + nums: ['median: 2', 'q1: 1.5', 'q3: 2.5'], name: ['', '', ''], axis: 'trace 0' }].forEach(function(specs) { diff --git a/test/jasmine/tests/violin_test.js b/test/jasmine/tests/violin_test.js index ab4bb5ed657..263761af559 100644 --- a/test/jasmine/tests/violin_test.js +++ b/test/jasmine/tests/violin_test.js @@ -388,8 +388,8 @@ describe('Test violin hover:', function() { }, nums: [ 'q3: 0.6', 'median: 0.45', 'q3: 0.6', 'max: 1', 'y: 0.9266848, kde: 0.383', - 'median: 0.55', 'min: 0', 'q1: 0.3', 'min: 0.2', 'max: 0.7', 'y: 0.9266848, kde: 0.182', - 'median: 0.45', 'min: 0.1', 'q3: 0.6', 'max: 0.9', 'y: 0.9266848, kde: 0.435' + 'median: 0.55', 'min: 0', 'q1: 0.3', 'q1: 0.2', 'max: 0.7', 'y: 0.9266848, kde: 0.182', + 'median: 0.45', 'q1: 0.1', 'q3: 0.6', 'max: 0.9', 'y: 0.9266848, kde: 0.435' ], name: [ '', 'kale', '', '', '', 'radishes', '', '', '', '', @@ -535,7 +535,7 @@ describe('Test violin hover:', function() { name: ['', '', '', '', '', ''], axis: 'Sat', hoverLabelPos: [ - [364, 270], [339, 270], [352, 270], + [364, 270], [352, 270], [339, 270], [346, 270], [349, 270], [387, 270] ] }, { From ce5e5f96d58e8f8b9a1f8947e7b38344d5507977 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 14 Jun 2019 16:48:34 -0400 Subject: [PATCH 4/4] add 'attr' key to box/hover pointData - this makes all box/hover labels render even when their x/y positions overlap perfectly. --- src/traces/box/hover.js | 1 + test/jasmine/tests/box_test.js | 8 +++----- test/jasmine/tests/violin_test.js | 2 +- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/traces/box/hover.js b/src/traces/box/hover.js index 06936675f29..0eac65f7669 100644 --- a/src/traces/box/hover.js +++ b/src/traces/box/hover.js @@ -171,6 +171,7 @@ function hoverOnBoxes(pointData, xval, yval, hovermode) { var valPx = vAxis.c2p(val, true); var pointData2 = Lib.extendFlat({}, pointData); + pointData2.attr = attr; pointData2[vLetter + '0'] = pointData2[vLetter + '1'] = valPx; pointData2[vLetter + 'LabelVal'] = val; pointData2[vLetter + 'Label'] = (t.labels ? t.labels[attr] + ' ' : '') + Axes.hoverLabelText(vAxis, val); diff --git a/test/jasmine/tests/box_test.js b/test/jasmine/tests/box_test.js index ef291630683..998f32ee3cb 100644 --- a/test/jasmine/tests/box_test.js +++ b/test/jasmine/tests/box_test.js @@ -438,23 +438,21 @@ describe('Test box hover:', function() { nums: '0.6', name: 'pt #0' }, { - desc: 'when zoomed in, cropping out labels', + desc: 'when zoomed in, within q1-q3 making min/q1 and max/q3 overlap', mock: { data: [{ type: 'box', y: [1, 2, 2, 3] }], layout: { - // cropping out q1 and max w/o failing during hoverAvoidOverlap - // https://github.com/plotly/plotly.js/issues/3962 yaxis: {range: [1.6, 2.4]}, width: 400, height: 400 } }, pos: [200, 200], - nums: ['median: 2', 'q1: 1.5', 'q3: 2.5'], - name: ['', '', ''], + nums: ['median: 2', 'q1: 1.5', 'q3: 2.5', 'max: 3', 'min: 1'], + name: ['', '', '', '', ''], axis: 'trace 0' }].forEach(function(specs) { it('should generate correct hover labels ' + specs.desc, function(done) { diff --git a/test/jasmine/tests/violin_test.js b/test/jasmine/tests/violin_test.js index 263761af559..ac794a36b02 100644 --- a/test/jasmine/tests/violin_test.js +++ b/test/jasmine/tests/violin_test.js @@ -745,7 +745,7 @@ describe('Test violin hover:', function() { actual = actual.sort(function(a, b) { return a[1].top - b[1].top; }); - expect(actual.length).toBe(7, '# of value hover labels'); + expect(actual.length).toBe(8, '# of value hover labels'); for(var i = 0; i < actual.length - 1; i++) { var a = actual[i];