Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support template string on hover #3126

Merged
merged 53 commits into from
Nov 15, 2018
Merged

support template string on hover #3126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
53 commits
Select commit Hold shift + click to select a range
4c197e8
rough first implementation for hovertemplate
antoinerg Oct 15, 2018
613c8f5
supply hover's eventData inot hovertemplate
antoinerg Oct 16, 2018
edf4795
delete unrelated file from branch
antoinerg Oct 17, 2018
dd62855
update description for hovertemplate
antoinerg Oct 17, 2018
a86327a
fix hovertemplate test not being run
antoinerg Oct 18, 2018
1fde3aa
for now, only offer hoverdata and trace objects to hovertemplate
antoinerg Oct 18, 2018
d263cce
templateString evaluates attributes with a dot in their name
antoinerg Oct 18, 2018
040208f
do not coerce hoverinfo if hovertemplate is defined
antoinerg Nov 5, 2018
3eaa3b9
replace templateString with the more specific hovertemplateString
antoinerg Nov 5, 2018
0084efb
hovertemplate: add warning if variable can't be found
antoinerg Nov 5, 2018
046d367
coerce hovertemplate at trace-level, starting with scatter(gl)
antoinerg Nov 6, 2018
59083ad
add <extra></extra> tag to hovertemplate for secondary labels
antoinerg Nov 6, 2018
266d43a
add URL to d3-format documentation
antoinerg Nov 6, 2018
108b68a
initial hovertemplate support for pie
antoinerg Nov 6, 2018
eb1a94a
pie hovertemplate test %{label}
antoinerg Nov 7, 2018
8214d36
bar support with limited test
antoinerg Nov 7, 2018
bb97abe
add hovertemplate support in histogram
antoinerg Nov 7, 2018
22e3a9d
pie returns default formatted value
antoinerg Nov 8, 2018
377ecba
pass hovertemplate data around in `hoverData` instead of opts
antoinerg Nov 8, 2018
bc6cbab
update Fx.multiHovers to properly massage hoverItem
antoinerg Nov 8, 2018
8c9ba5b
fix lint
antoinerg Nov 8, 2018
c767482
pass `trace` object in Fx.loneHover and Fx.multiHovers for hovertemplate
antoinerg Nov 8, 2018
6d4c03a
extra regex still matches in the presence of newlines
antoinerg Nov 8, 2018
8f440b0
fix jsDocs syntax
antoinerg Nov 12, 2018
0659d20
move regex to outer scope
antoinerg Nov 12, 2018
41ab464
remove old unused commented lines
antoinerg Nov 12, 2018
37e40f9
move regex to outside scope
antoinerg Nov 12, 2018
8e8b75b
move hovertemplate out of global-level plots attributes
antoinerg Nov 12, 2018
2015c57
scatter: do not coerce hovertemplate if hoveron: 'fills'
antoinerg Nov 12, 2018
aef48c0
fix lint
antoinerg Nov 12, 2018
a3058f4
test hovertemplate support for <extra> and pseudo-html
antoinerg Nov 12, 2018
3068d7d
hovertemplate warns user about missing variables up to 10 times
antoinerg Nov 12, 2018
a808883
hovertemplate attribute supports array
antoinerg Nov 12, 2018
522f744
scattergl support for hovertemplate array
antoinerg Nov 12, 2018
c501b44
pie support for hovertemplate array
antoinerg Nov 12, 2018
f89baca
make axes available in eventData to give access to its title
antoinerg Nov 12, 2018
369c9d6
add axis information to eventData only if hovertemplate
antoinerg Nov 12, 2018
8d95f05
axis information is already included in hovertemplate
antoinerg Nov 12, 2018
1e4bd33
pie: test that hovertemplate supports array
antoinerg Nov 13, 2018
61a2da7
list available variables in pie's hovertemplate description
antoinerg Nov 13, 2018
43a4cd9
hovertemplate: do not look into trace object, use fullData instead
antoinerg Nov 13, 2018
70befe3
describe hovertemplate variables for scatter(gl)
antoinerg Nov 13, 2018
b6822e8
describe hovertemplate variables for histogram
antoinerg Nov 13, 2018
59bc981
fix lint
antoinerg Nov 13, 2018
f75f2e0
scatter: test hover event data
antoinerg Nov 13, 2018
0654245
test that event data has correct fields in bar, scatter, histogram
antoinerg Nov 13, 2018
0ecd1c1
bar test: fix hover position to trigger hover events
antoinerg Nov 13, 2018
6f86522
scatter: add `marker.color` to `hovertemplate` available variables
antoinerg Nov 14, 2018
7be804e
one source of truth for event data keys used for doc and test
antoinerg Nov 14, 2018
583eb97
fix lint
antoinerg Nov 14, 2018
14ac99f
scatter: list additionnal variables available in eventData
antoinerg Nov 14, 2018
1caf321
hovertemplate: update desc, do no list attributes that are `arrayOK`
antoinerg Nov 15, 2018
a78600a
fix syntax
antoinerg Nov 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/traces/scatter/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ module.exports = {
].join(' ')
},
hovertemplate: hovertemplateAttrs({}, {
keys: ['marker.size', 'marker.color']
keys: ['marker.size']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is marker.color gone? In fact, all arrayOk attributes should be part of this list. They get added to the event data via

plotly.js/src/components/fx/helpers.js

Lines 161 to 186 in 2ceb5c3

/** Appends values inside array attributes corresponding to given point number
*
* @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;
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 pointVal = getPointData(val, pointNumber);
if(pointVal !== undefined) pointData[key] = pointVal;
}
}
};

Now, maybe we don't need to hardcode all the arrayOk attributes into keys here, maybe we could just mention that all "per-point" values are available for hovertemplate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

marker.color is gone because it is not emitted in the event data. It used to work because hovertemplateString would be called with the full trace object as one of its argument but I removed this in 43a4cd9 to make sure everything is on par with event data. Therefore, if we want marker.color, I need to add it to the event data for scatter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

marker.color is gone because it is not emitted in the event data

Try

Plotly.newPlot(gd, [{
  y: [1, 2, 3],
  marker: {color: [1, 2, 3]}
}])
gd.on('plotly_hover', console.log)

Copy link
Contributor Author

@antoinerg antoinerg Nov 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works if it's an array but not if it isn't? 🤔

Plotly.newPlot(gd, [{
  y: [1, 2, 3],
  marker: {color: 'blue'}
}])
gd.on('plotly_hover', console.log)

That's why the test was failing: the mock I was using doesn't specify marker.color as an array.

I think it should be available in both cases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t works if it's an array but not if it isn't?

yes, that's the expected behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so I added it and now run the test with a mock that specifies marker size and color as arrays. And it now passes.

Now for extra points, I just need to add those extra event data keys in constants.js.

Copy link
Contributor Author

@antoinerg antoinerg Nov 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 7be804e

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized I didn't read your comment properly. I am sorry about that @etpinard.

Commit 14ac99f should be more satisfactory. I now list the different marker attributes that are arrayOk except for the nested line and gradient. Should I add those as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From that same #3126 (comment)

Now, maybe we don't need to hardcode all the arrayOk attributes into keys here,

I think it might be best to NOT hardcoded all arrayOk attributes into that keys list, as it will be hard to remember to append it everytime we add another arrayOk attribute. We should keep that keys list only for keys that have no corresponding schema attributes e.g binNumber in histogram traces, and we should mention that all arrayOk (aka per-point) attribute are available to hovertemplate in the components/fx/hovertemplate_attributes' description.

Sorry for not been clearer.

}),
line: {
color: {
Expand Down
40 changes: 40 additions & 0 deletions test/jasmine/tests/scatter_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ var transitions = require('../assets/transitions');
var assertClip = customAssertions.assertClip;
var assertNodeDisplay = customAssertions.assertNodeDisplay;
var assertMultiNodeOrder = customAssertions.assertMultiNodeOrder;
var hover = require('../assets/hover');

var getOpacity = function(node) { return Number(node.style.opacity); };
var getFillOpacity = function(node) { return Number(node.style['fill-opacity']); };
Expand Down Expand Up @@ -1808,3 +1809,42 @@ describe('Test scatter *clipnaxis*:', function() {
.then(done);
});
});

describe('event data', function() {
var mock = require('@mocks/12');
var mockCopy = Lib.extendDeep({}, mock),
gd;

beforeEach(function(done) {
gd = createGraphDiv();

Plotly.plot(gd, mockCopy.data, mockCopy.layout)
.then(done);
});

afterEach(destroyGraphDiv);

it('should contain the correct fields', function() {

var hoverData;

gd.on('plotly_hover', function(data) {
hoverData = data;
});

hover(130, 350);

expect(hoverData.points.length).toEqual(1);

var fields = [
'curveNumber', 'pointNumber',
'data', 'fullData',
'xaxis', 'yaxis', 'x', 'y',
'marker.size'
];

fields.forEach(function(field) {
expect(Object.keys(hoverData.points[0])).toContain(field);
});
});
});