Pie aggregation and event cleanup #2117
+554
−455
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -89,7 +89,8 @@ exports.quadrature = function quadrature(dx, dy) { | ||
| * | ||
| * @param {object} pointData : point data object (gets mutated here) | ||
| * @param {object} trace : full trace object | ||
| * @param {number|Array(number)} pointNumber : point number. May be a length-2 array | ||
| * [row, col] to dig into 2D arrays | ||
| */ | ||
| exports.appendArrayPointValue = function(pointData, trace, pointNumber) { | ||
| var arrayAttrs = trace._arrayAttrs; | ||
| @@ -100,22 +101,68 @@ exports.appendArrayPointValue = function(pointData, trace, pointNumber) { | ||
|
|
||
| for(var i = 0; i < arrayAttrs.length; i++) { | ||
| var astr = arrayAttrs[i]; | ||
| var key = getPointKey(astr); | ||
|
|
||
| if(pointData[key] === undefined) { | ||
| var val = Lib.nestedProperty(trace, astr).get(); | ||
| var pointVal = getPointData(val, pointNumber); | ||
|
|
||
| if(pointVal !== undefined) pointData[key] = pointVal; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * Appends values inside array attributes corresponding to given point number array | ||
| * For use when pointData references a plot entity that arose (or potentially arose) | ||
| * from multiple points in the input data | ||
| * | ||
| * @param {object} pointData : point data object (gets mutated here) | ||
| * @param {object} trace : full trace object | ||
| * @param {Array(number)|Array(Array(number))} pointNumbers : Array of point numbers. | ||
| * Each entry in the array may itself be a length-2 array [row, col] to dig into 2D arrays | ||
| */ | ||
| exports.appendArrayMultiPointValues = function(pointData, trace, pointNumbers) { | ||
| var arrayAttrs = trace._arrayAttrs; | ||
|
|
||
| if(!arrayAttrs) { | ||
| return; | ||
| } | ||
|
|
||
| for(var i = 0; i < arrayAttrs.length; i++) { | ||
| var astr = arrayAttrs[i]; | ||
| var key = getPointKey(astr); | ||
|
|
||
| if(pointData[key] === undefined) { | ||
| var val = Lib.nestedProperty(trace, astr).get(); | ||
| var keyVal = new Array(pointNumbers.length); | ||
|
|
||
| for(var j = 0; j < pointNumbers.length; j++) { | ||
| keyVal[j] = getPointData(val, pointNumbers[j]); | ||
| } | ||
| pointData[key] = keyVal; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| var pointKeyMap = { | ||
| ids: 'id', | ||
| locations: 'location', | ||
| labels: 'label', | ||
| values: 'value', | ||
| 'marker.colors': 'color' | ||
| }; | ||
|
|
||
| function getPointKey(astr) { | ||
| return pointKeyMap[astr] || astr; | ||
| } | ||
|
|
||
| function getPointData(val, pointNumber) { | ||
|
There was a problem hiding this comment. This thing is very similar to There was a problem hiding this comment. For the multipoint case it's nice to keep the There was a problem hiding this comment. Right, it might be nice to move |
||
| if(Array.isArray(pointNumber)) { | ||
| if(Array.isArray(val) && Array.isArray(val[pointNumber[0]])) { | ||
| return val[pointNumber[0]][pointNumber[1]]; | ||
| } | ||
| } else { | ||
| return val[pointNumber]; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -19,13 +19,14 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout | ||
| var coerceFont = Lib.coerceFont; | ||
|
|
||
| var vals = coerce('values'); | ||
| var labels = coerce('labels'); | ||
| if(!Array.isArray(labels)) { | ||
| if(!Array.isArray(vals) || !vals.length) { | ||
|
There was a problem hiding this comment. Blocking: Would you mind locking this down in a jasmine test? There was a problem hiding this comment. sure -> ee1f8c4 |
||
| // must have at least one of vals or labels | ||
| traceOut.visible = false; | ||
| return; | ||
| } | ||
|
|
||
| coerce('label0'); | ||
| coerce('dlabel'); | ||
| } | ||
| @@ -34,14 +35,10 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout | ||
| if(lineWidth) coerce('marker.line.color'); | ||
|
|
||
| var colors = coerce('marker.colors'); | ||
| if(!Array.isArray(colors)) traceOut.marker.colors = []; | ||
|
|
||
| coerce('scalegroup'); | ||
| // TODO: hole needs to be coerced to the same value within a scaleegroup | ||
|
|
||
| var textData = coerce('text'); | ||
| var textInfo = coerce('textinfo', Array.isArray(textData) ? 'text+percent' : 'percent'); | ||
| @@ -63,14 +60,6 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout | ||
| coerce('domain.x'); | ||
| coerce('domain.y'); | ||
|
|
||
| coerce('hole'); | ||
|
|
||
| coerce('sort'); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /** | ||
| * Copyright 2012-2017, Plotly, Inc. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
|
|
||
| 'use strict'; | ||
|
|
||
| var appendArrayMultiPointValues = require('../../components/fx/helpers').appendArrayMultiPointValues; | ||
|
|
||
|
|
||
| // Note: like other eventData routines, this creates the data for hover/unhover/click events | ||
| // but it has a different API and goes through a totally different pathway. | ||
| // So to ensure it doesn't get misused, it's not attached to the Pie module. | ||
| module.exports = function eventData(pt, trace) { | ||
| var out = { | ||
| curveNumber: trace.index, | ||
| pointNumbers: pt.pts, | ||
| data: trace._input, | ||
| fullData: trace, | ||
| label: pt.label, | ||
| color: pt.color, | ||
| value: pt.v, | ||
|
|
||
| // pt.v (and pt.i below) for backward compatibility | ||
| v: pt.v | ||
| }; | ||
|
|
||
| // Only include pointNumber if it's unambiguous | ||
| if(pt.pts.length === 1) out.pointNumber = out.i = pt.pts[0]; | ||
|
|
||
| // Add extra data arrays to the output | ||
| // notice that this is the multi-point version ('s' on the end!) | ||
| // so added data will be arrays matching the pointNumbers array. | ||
| appendArrayMultiPointValues(out, trace, pt.pts); | ||
|
|
||
| return out; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| { | ||
| "data": [ | ||
| { | ||
| "labels": [ | ||
| "Alice", | ||
| "Bob", | ||
| "Charlie", | ||
| "Charlie", | ||
| "Charlie", | ||
| "Alice" | ||
| ], | ||
| "type": "pie", | ||
| "domain": {"x": [0, 0.4]} | ||
| }, | ||
| { | ||
| "labels": [ | ||
| "Alice", | ||
| "Alice", | ||
| "Allison", | ||
| "Alys", | ||
| "Elise", | ||
| "Allison", | ||
| "Alys", | ||
| "Alys" | ||
| ], | ||
| "values": [ | ||
| 10, 20, 30, 40, 50, 60, 70, 80 | ||
| ], | ||
| "marker": {"colors": ["", "", "#ccc"]}, | ||
| "type": "pie", | ||
| "domain": {"x": [0.5, 1]}, | ||
| "textinfo": "label+value+percent" | ||
| } | ||
| ], | ||
| "layout": { | ||
| "height": 300, | ||
| "width": 500 | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -65,6 +65,33 @@ describe('Pie traces:', function() { | ||
| .catch(failTest) | ||
| .then(done); | ||
| }); | ||
|
|
||
| it('can sum values or count labels', function(done) { | ||
| Plotly.newPlot(gd, [{ | ||
| labels: ['a', 'b', 'c', 'a', 'b', 'a'], | ||
| values: [1, 2, 3, 4, 5, 6], | ||
| type: 'pie', | ||
| domain: {x: [0, 0.45]} | ||
| }, { | ||
| labels: ['d', 'e', 'f', 'd', 'e', 'd'], | ||
| type: 'pie', | ||
| domain: {x: [0.55, 1]} | ||
| }]) | ||
| .then(function() { | ||
| var expected = [ | ||
| [['a', 11], ['b', 7], ['c', 3]], | ||
| [['d', 3], ['e', 2], ['f', 1]] | ||
| ]; | ||
| for(var i = 0; i < 2; i++) { | ||
| for(var j = 0; j < 3; j++) { | ||
| expect(gd.calcdata[i][j].label).toBe(expected[i][j][0], i + ',' + j); | ||
| expect(gd.calcdata[i][j].v).toBe(expected[i][j][1], i + ',' + j); | ||
| } | ||
| } | ||
| }) | ||
| .catch(failTest) | ||
| .then(done); | ||
| }); | ||
| }); | ||
|
|
||
| describe('pie hovering', function() { | ||
| @@ -141,24 +168,6 @@ describe('pie hovering', function() { | ||
|
|
||
| it('should contain the correct fields', function() { | ||
|
|
||
| var hoverData, | ||
| unhoverData; | ||
|
|
||
| @@ -178,19 +187,17 @@ describe('pie hovering', function() { | ||
| expect(unhoverData.points.length).toEqual(1); | ||
|
|
||
| var fields = [ | ||
| 'curveNumber', 'pointNumber', 'pointNumbers', | ||
| 'data', 'fullData', | ||
| 'label', 'color', 'value', | ||
| 'i', 'v' | ||
| ]; | ||
|
|
||
| expect(Object.keys(hoverData.points[0]).sort()).toEqual(fields.sort()); | ||
| expect(hoverData.points[0].pointNumber).toEqual(3); | ||
|
|
||
| expect(Object.keys(unhoverData.points[0]).sort()).toEqual(fields.sort()); | ||
| expect(unhoverData.points[0].pointNumber).toEqual(3); | ||
| }); | ||
|
|
||
| it('should fire hover event when moving from one slice to another', function(done) { | ||
| @@ -355,7 +362,7 @@ describe('pie hovering', function() { | ||
| }); | ||
|
|
||
|
|
||
| describe('Test event data of interactions on a pie plot:', function() { | ||
| var mock = require('@mocks/pie_simple.json'); | ||
|
|
||
| var mockCopy, gd; | ||
| @@ -376,10 +383,37 @@ describe('Test event property of interactions on a pie plot:', function() { | ||
| beforeEach(function() { | ||
| gd = createGraphDiv(); | ||
| mockCopy = Lib.extendDeep({}, mock); | ||
| Lib.extendFlat(mockCopy.data[0], { | ||
| ids: ['marge', 'homer', 'bart', 'lisa', 'maggie'], | ||
| customdata: [{1: 2}, {3: 4}, {5: 6}, {7: 8}, {9: 10}] | ||
|
There was a problem hiding this comment. Nice. I was wondering if |
||
| }); | ||
| }); | ||
|
|
||
| afterEach(destroyGraphDiv); | ||
|
|
||
| function checkEventData(data) { | ||
|
There was a problem hiding this comment. A long-overdue test abstraction. Thanks! |
||
| var point = data.points[0]; | ||
|
|
||
| expect(point.curveNumber).toBe(0); | ||
| expect(point.pointNumber).toBe(4); | ||
| expect(point.pointNumbers).toEqual([4]); | ||
| expect(point.data).toBe(gd.data[0]); | ||
| expect(point.fullData).toBe(gd._fullData[0]); | ||
| expect(point.label).toBe('4'); | ||
| expect(point.value).toBe(5); | ||
| expect(point.color).toBe('#1f77b4'); | ||
| expect(point.id).toEqual(['maggie']); | ||
| expect(point.customdata).toEqual([{9: 10}]); | ||
|
|
||
| // for backward compat - i/v to be removed at some point? | ||
|
There was a problem hiding this comment. Blocking: yeah, we should remove that in There was a problem hiding this comment. commented #420 (comment) |
||
| expect(point.i).toBe(point.pointNumber); | ||
| expect(point.v).toBe(point.value); | ||
|
|
||
| var evt = data.event; | ||
| expect(evt.clientX).toEqual(pointPos[0], 'event.clientX'); | ||
| expect(evt.clientY).toEqual(pointPos[1], 'event.clientY'); | ||
| } | ||
|
|
||
| describe('click events', function() { | ||
| var futureData; | ||
|
|
||
| @@ -400,43 +434,24 @@ describe('Test event property of interactions on a pie plot:', function() { | ||
| click(pointPos[0], pointPos[1]); | ||
| expect(futureData.points.length).toEqual(1); | ||
|
|
||
|
There was a problem hiding this comment.
And yet we had a test to verify that that's exactly what we did 🤔 |
||
| checkEventData(futureData); | ||
| }); | ||
|
|
||
| it('should not contain pointNumber if aggregating', function() { | ||
| var values = gd.data[0].values; | ||
| var labels = []; | ||
| for(var i = 0; i < values.length; i++) labels.push(i); | ||
| Plotly.restyle(gd, { | ||
| labels: [labels.concat(labels)], | ||
| values: [values.concat(values)] | ||
| }); | ||
|
|
||
| click(pointPos[0], pointPos[1]); | ||
| expect(futureData.points.length).toEqual(1); | ||
|
|
||
| expect(futureData.points[0].pointNumber).toBeUndefined(); | ||
| expect(futureData.points[0].i).toBeUndefined(); | ||
| expect(futureData.points[0].pointNumbers).toEqual([4, 9]); | ||
| }); | ||
| }); | ||
|
|
||
| @@ -466,42 +481,9 @@ describe('Test event property of interactions on a pie plot:', function() { | ||
| click(pointPos[0], pointPos[1], clickOpts); | ||
| expect(futureData.points.length).toEqual(1); | ||
|
|
||
| checkEventData(futureData); | ||
|
|
||
| var evt = futureData.event; | ||
| Object.getOwnPropertyNames(clickOpts).forEach(function(opt) { | ||
| expect(evt[opt]).toEqual(clickOpts[opt], 'event.' + opt); | ||
| }); | ||
| @@ -512,7 +494,8 @@ describe('Test event property of interactions on a pie plot:', function() { | ||
| var futureData; | ||
|
|
||
| beforeEach(function(done) { | ||
| futureData = undefined; | ||
| Plotly.newPlot(gd, mockCopy.data, mockCopy.layout).then(done); | ||
|
|
||
| gd.on('plotly_hover', function(data) { | ||
| futureData = data; | ||
| @@ -522,33 +505,44 @@ describe('Test event property of interactions on a pie plot:', function() { | ||
| it('should contain the correct fields', function() { | ||
| mouseEvent('mouseover', pointPos[0], pointPos[1]); | ||
|
|
||
| checkEventData(futureData); | ||
| }); | ||
|
|
||
| it('should not emit a hover if you\'re dragging', function() { | ||
| gd._dragging = true; | ||
|
There was a problem hiding this comment. It occurred to me when fixing this bit of logic, it would perhaps be better to check There was a problem hiding this comment. that will need a bunch more testing and ideally rolling out to all subplots at once, so I'll defer it -> #2120 |
||
| mouseEvent('mouseover', pointPos[0], pointPos[1]); | ||
| expect(futureData).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should not emit a hover if hover is disabled', function() { | ||
| Plotly.relayout(gd, 'hovermode', false); | ||
| mouseEvent('mouseover', pointPos[0], pointPos[1]); | ||
| expect(futureData).toBeUndefined(); | ||
|
There was a problem hiding this comment. This is also different from before, but consistent with other subplot types, I believe. There was a problem hiding this comment. Yes this is how it should be. For the record, to get hover event data w/o visible hover labels, one should set |
||
| }); | ||
| }); | ||
|
|
||
| describe('unhover events', function() { | ||
| var futureData; | ||
|
|
||
| beforeEach(function(done) { | ||
| futureData = undefined; | ||
| Plotly.newPlot(gd, mockCopy.data, mockCopy.layout).then(done); | ||
|
|
||
| gd.on('plotly_unhover', function(data) { | ||
| futureData = data; | ||
| }); | ||
| }); | ||
|
|
||
| it('should contain the correct fields', function() { | ||
| mouseEvent('mouseover', pointPos[0], pointPos[1]); | ||
| mouseEvent('mouseout', pointPos[0], pointPos[1]); | ||
|
|
||
| checkEventData(futureData); | ||
| }); | ||
|
|
||
| it('should not emit an unhover if you didn\'t first hover', function() { | ||
| mouseEvent('mouseout', pointPos[0], pointPos[1]); | ||
| expect(futureData).toBeUndefined(); | ||
|
There was a problem hiding this comment. kind of a weird one... but lets say you dragged over the pie from another subplot, so there was no hover event - you shouldn't get an unhover event even if you mouse up and then mouse out of that slice. |
||
| }); | ||
| }); | ||
| }); | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ at some point we might want to move this to the attribute declarations.