-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
Conversation
src/components/fx/hover.js
Outdated
// hovertemplate | ||
var trace = d.trace, hovertemplate = opts.hovertemplate || trace.hovertemplate || false; | ||
if(hovertemplate) { | ||
text = Lib.templateString(hovertemplate, gd._hoverdata[curveNumber], trace); |
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.
We can pass additional objects as argument to Lib.templateString
so the sky is the limit in terms of the number of values we can make available here!
However, as mentioned in point 1, we should probably put useful data into _hoverdata
to begin with since it's emitted on hover.
@antoinerg You should consider adding a bunch of examples to plotly/documentation to demonstrate the new possibilities with this. |
Matching event data We'll need to handle traces that support a
Thanks writing down thoughts about traces that can generate multiple hover labels. I agree, we'll need to implement per-label templating and styling as some point. In the case of
Maybe these special tags could help us out. Consider:
would show three labels: one for the mean, q1 and q3 when hovering on a box. The mean hover label would have the trace "name" attach to it in a secondary label and its text would drawn in blue. Now, those Let me write down a few less-important comments here about the implementation:
|
|
The box-like case (incl. violin, ohlc, candlestick) seems like the toughest nut to crack. There's also the issue of whether you make one or many labels, ie
Again as I mentioned earlier, I like the plan of describing each class of hover label within its own container. The difficult case for this currently is scatter |
👍
Good plan. Related: #420 (comment) |
now in -> #3203 |
Is there ever a case where this is not simply a mistake the user will want to know about and fix? If not, I'd say leave it
👍 I do think there's a possibility of having
I like the idea of this being the default plotly formatting. That way we don't need to augment the d3 format spec at all. I don't see a benefit to raw.
👍
interesting... sure, perhaps in conjunction with the effort to have |
Great - thanks @alexcjohnson I think all my concerns have been resolved (at least for this first iteration). @antoinerg should be all set now 🎉 |
'hovertemplate %{percent}' | ||
); | ||
|
||
return Plotly.restyle(gd, 'hovertemplate', '%{label}<extra></extra>'); |
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.
Could we add one arrayOk hovertemplate
test for pies?
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.
Absolutely! There it is 1e4bd33
src/traces/bar/attributes.js
Outdated
@@ -59,6 +60,7 @@ module.exports = { | |||
|
|||
text: scatterAttrs.text, | |||
hovertext: scatterAttrs.hovertext, | |||
hovertemplate: hovertemplateAttrs(), |
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.
We should append the hovertemplate
descriptions with a list of flags (i.e. event data keys) available.
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.
On a related note: I like that for some traces, like pie
, we test that event data has the correct keys:
plotly.js/test/jasmine/tests/pie_test.js
Lines 824 to 856 in ad937d2
it('should contain the correct fields', function() { | |
var hoverData, | |
unhoverData; | |
gd.on('plotly_hover', function(data) { | |
hoverData = data; | |
}); | |
gd.on('plotly_unhover', function(data) { | |
unhoverData = data; | |
}); | |
mouseEvent('mouseover', width / 2 - 7, height / 2 - 7); | |
mouseEvent('mouseout', width / 2 - 7, height / 2 - 7); | |
expect(hoverData.points.length).toEqual(1); | |
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); | |
}); |
We could potentially use the same array of field names for the test and for the description making sure we have one source of truth 🤔. This improvement could be done in another PR.
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.
We could potentially use the same array of field names for the test and for the description making sure we have one source of truth
Yeah, good call. We usually put things like that in "constants" files like this one:
https://github.com/plotly/plotly.js/blob/master/src/traces/scatter/constants.js
This improvement could be done in another PR.
That's up to you!
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.
I created a new function to check that event data contains a given list of keys: https://github.com/plotly/plotly.js/pull/3126/files#diff-4ddfa80f2c61e112d5587c91a961ab47
If it looks OK, I will dry up the code by listing the extra keys of a given trace into its constants.js
file and then use that list to generate the description of hovertemplate
attribute 🎉 !
Let me know if that route looks fine to you.
Getting close, once: are ✅ , I believe we'll be in 💃 land. |
src/traces/scatter/attributes.js
Outdated
@@ -205,7 +205,7 @@ module.exports = { | |||
].join(' ') | |||
}, | |||
hovertemplate: hovertemplateAttrs({}, { | |||
keys: ['marker.size', 'marker.color'] | |||
keys: ['marker.size'] |
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.
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
.
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.
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
.
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.
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)
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.
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.
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.
t works if it's an array but not if it isn't?
yes, that's the expected behaviour.
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.
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
.
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.
Done in 7be804e
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.
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.
From that same #3126 (comment)
Now, maybe we don't need to hardcode all the
arrayOk
attributes intokeys
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.
Beautifully done. 💃 |
Nice example of this new
Codepen: https://codepen.io/nicolaskruchten/pen/QJgbxo?editors=0010 |
Maybe stupid question: Is it possible to style the hovertemplate more? Like having a table structure? |
No. That'll be in #3010 |
Closes #3007
See this #3126 (comment) for understanding how to add support for
hovertemplate
to new traces.However, there are several caveats:
1 - In the process of making that PR, I realized that there are a lot of computed values on hover that are not emitted in hover events. Because we are passing the hover event data to the template, only a limited subset of the total information is currently available. Enriching the event data for each trace is probably outside the scope of the current PR since it should benefit BOTH
hovertemplate
andplotly_hover
events. However, it is something that can be done fairly easily and separately (some traces already have anevent_data.js
file in place where such code would go)!2 - Some traces have many hover labels. For example, in
box
traces, there are six labels when hovering on a box. As of right now, the numerical value for each of those is assigned to variabley
regardless of what it represents (q(1|3)
,min
,max
, etc.). We should certainly add alabel
variable alongside in order for template'%{label}: %{y}'
to exactly reproduce the current hover labels while enabling number formatting. All traces with several different hover labels would have to implement this change.In the future, to support different templates for each hover labels, we could either allow
hovertemplate
to be an object with a key for each label ({q1: 'templatestring', min: 'templatestring', ...}
) OR we can move everything in nested attributes :{q1: {hovertemplate: '...'}, min: {hovertemplate: '...'}, ...}
. The latter has the advantages of allowing further customization for each label (think color or hiding) since each of them could have a nestedhoverinfo
andhoverlabel
. We need to think carefully about this one. The latter approach is what @alexcjohnson and I decided to do in our latest work forsankey
traces (see PR #3096).3 - Right now,
hovertemplate
doesn't control the content of the secondary labels. As @alexcjohnson pointed out, this could be supported by a special tag<extra>Secondary box content here</extra>
.Please chime in :)