Conversation
- allow arrayOk for all hover label attribute - don't set dflt for bgcolor, bordercolor and font.color as their value depends and the trace color and plot bgcolor
- to that there (as opposed to in trace module hoverPoints) to have to repeat this logic across all trace modules - note that PR #1573 already added the relevant `createHoverText` logic
My gut reaction is to make
Absolutely. Dunno if now is the time to do it, may be a bit of a project, but if you're excited about it now then by all means go for it. |
I could understand some users wanting the data and the axis labels have the same style. I can see a case for Also +1 from for
What Alex said (+1 from me too) |
|
OK. I'll add layout: {
hoverlabel: {
bgcolor: 'white',
font: {
size: 20,
color: 'black'
}
}
}
data = [{
y: [/* ... */],
hoverlabel: {
font: {
color: 'biue'
}
}
}]would make all hover labels have a white bg, common labels will have black text and trace labels blue text. Sounds ok? Then, if someone really wants per-axis common label customizable down the road, we can always add |
+1 to white bg |
I was thinking the colors in But @n-riesco raises a good point:
That would be annoying the way I'm suggesting, as it would require explicitly coloring all the trace labels. I suppose we could make some boolean attribute like
👍 |
|
@alexcjohnson @etpinard I really misunderstood Please, correct me if I'm mistaken. This is how understand
Oh, I missed that at the moment data hover labels are coloured after the marker!
When I wrote that, what I had in mind was a bar chart (each bar with different colours). Currently bar charts have axis hover labels with a black background, whereas data hover labels are coloured after the bar. The use case I had in mind was that both axis and data hover labels were coloured after the bar. How about having |
When there's a common label it's because you're in a hover mode (ie compare) that supports multiple labels... so this wouldn't be meaningful in general. |
- into component/fx/ - keep old 'init' routine in graph_interact.js - mv hover constants out of cartesian constants.js and into fx/
- this removes one more circular dep!
Fx component
- to coerce hoverlabel container in a DRY way
- somehow `madge` identifies circular in an order-dependent way, bump back to pre PR #1613 value.
tasks/test_syntax.js
Outdated
| @@ -177,7 +177,7 @@ function assertCircularDeps() { | |||
| var logs = []; | |||
|
|
|||
| // see https://github.com/plotly/plotly.js/milestone/9 | |||
| var MAX_ALLOWED_CIRCULAR_DEPS = 12; | |||
| var MAX_ALLOWED_CIRCULAR_DEPS = 17; | |||
There was a problem hiding this comment.
I got a little too excited in #1613, that PR didn't remove any circular deps. It only made madge think that there were less ciruclar deps (their algo must order-dependent).
If you don't believe me, try adding var Fx = require('../fx'); in annotations_defaults like here and you get back the 12 circular deps of PR #1613 .
|
@alexcjohnson this PR is ready for review. It's probably best to review it commit by commit after the #1613 merge commit. To note, |
- to test main codepath instead of fallback `ax._length`
- so that e.g. the hover label `color` field isn't overridden by undefined `hoverlabel.bgcolor` values.
- and all (future) plots/attributes.js declarations.
src/components/fx/helpers.js
Outdated
|
|
||
| // convenience functions for mapping all relevant axes | ||
| exports.flat = function flat(subplots, v) { | ||
| var out = []; |
There was a problem hiding this comment.
faster (here and in p2c below) to do var out = new Array(subplots.length) and avoid push?
src/components/fx/hover.js
Outdated
| .style({fill: Color.defaultLine, 'stroke-width': '1px', stroke: Color.background}); | ||
| .style({ | ||
| fill: commonLabelOpts.bgcolor || Color.defaultLine, | ||
| stroke: commonLabelOpts.bordercolor || Color.background, |
There was a problem hiding this comment.
might want to be Color.fill and Color.stroke since these I guess support alpha now
There was a problem hiding this comment.
I started looking around at other places this might be needed... and there are a bunch potentially (seems like all in this file), including some in spikelines that I didn't catch when reviewing that. I'm not sure if it's really crucial, the distinction is mainly important for export right? And you're not going to export hover effects. Still, best to be consistent.
There was a problem hiding this comment.
After some thought, I think it would be best to not render the alpha channel in all hover labels, as the marker hover labels already blend marker opacities with plot_bgcolor filtering out the alpha channel. For example,
var trace = {
x: [1],
y: [1],
marker: {
color: ['rgba(255, 0, 0, 0.3)']
}
}
var layout = {
plot_bgcolor: 'grey'
}renders the marker hover label as rgb(166, 90, 90) i.e. with no alpha channel.
We could add some special logic so that e.g.
var trace = {
x: [1],
y: [1],
marker: {
color: ['rgba(255, 0, 0, 0.3)']
},
hovelabel: {
bgcolor: 'rgba(0,0,255,0.3)'
}
}
var layout = {
plot_bgcolor: 'grey'
}renders the alpha channel overriding the marker color to hover label color behavior, but quite frankly I don't see the need.
Adding a line in the schema description saying that alpha channel aren't honored sounds sufficient for now. Thoughts?
There was a problem hiding this comment.
I've occasionally thought it would be nice to be able to see behind the hover labels - not just behind the name field like you can do now, but for example in a fractal when I want to draw a small zoom box it would be cool if the whole label were partially transparent. But in cases like that, seems like the best solution would be to make another attribute - hoverlabel.opacity, akin to marker.opacity so you don't need to set the background, the font color, and the border color all to rgba values - in fact, doing it that way would be a substandard experience for exactly the same reason as we have a marker.opacity in the first place, that you really want opacity applied to the whole group.
So yeah, I guess I'm agreeing with you that these attributes needn't support opacity on their own. If you feel like putting in hoverlabel.opacity now go for it, but it can also be left out of this PR and done later.
There was a problem hiding this comment.
but it can also be left out of this PR and done later.
I'm going to go with that if you don't mind. This PR is big enough.
src/plots/gl2d/scene2d.js
Outdated
| @@ -667,3 +675,18 @@ proto.hoverFormatter = function(axisName, val) { | |||
| var axis = this[axisName]; | |||
| return Axes.tickText(axis, axis.c2l(val), 'hover').text; | |||
| }; | |||
|
|
|||
| proto.castHoverOption = function(trace, ptNumber, attr) { | |||
There was a problem hiding this comment.
possible to share this with the one in gl3d? Looks nearly identical?
There was a problem hiding this comment.
It is possible. I didn't do it because as brought up in #1582 (comment), the real solution would be to replace Fx.loneHover by something more similar to Fx.hover in gl2d and gl3d. But, for the time being, might as well put this logic something in the Fx module object.
src/traces/sankey/plot.js
Outdated
| @@ -124,7 +124,8 @@ module.exports = function plot(gd, calcData) { | |||
| }; | |||
|
|
|||
| var linkHoverFollow = function(element, d) { | |||
|
|
|||
| var trace = gd._fullData[d.traceId]; | |||
| var ptNumber = d.originalIndex; | |||
There was a problem hiding this comment.
@monfera does this look ok to you? I'm looking for the data array index corresponding to this link / node (below).
Oh wait. node and link can have different lengths, correct? So maybe hoverlabel should be set per-node and per-link e.g.:
var trace = {
node: {
hoverlabel: {
bgcolor: [/* same length as node data arrays */]
},
link: {
hoverlabel: {
bgcolor: [/* same length as link data arrays */]
}
}
},
hoverlabel: {
// for trace-wide hover label settings (that would be applied to all nodes and links)
}
}Well the above sounds uselessly complicated for v1 of sankey. So, I'd vote for not allowing array values in hoverlabel attribute of sankey traces. Does that sound ok?
There was a problem hiding this comment.
Yes your conclusion sounds good to me. Indeed it'll be an unlikely occurrence that the number of nodes and links match.
There was a problem hiding this comment.
We can leave it off for now to get 1.27 out, but we should definitely include custom hovertext (and at that point we might as well include custom styling) for both nodes and links in the near future - I can definitely see people wanting to explain what specifically is included in each of these elements in more detail than is displayed permanently on the diagram. Chatting with @monfera seems like link.label fills this role for links but there's no analog for nodes. I'll make an issue for this so we don't forget about it.
There was a problem hiding this comment.
I'll make an issue for this so we don't forget about it.
Thanks!
test/jasmine/tests/sankey_test.js
Outdated
| return new Promise(function(resolve) { | ||
| setTimeout(resolve, 60); | ||
| }); | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
That thing got 🔪
I've been using this pattern lately. No need for timeouts 🎉
test/jasmine/tests/sankey_test.js
Outdated
|
|
||
| window.setTimeout(function() { | ||
| expect(d3.select('.hoverlayer>.hovertext>text').node().innerHTML) | ||
| .toEqual('46TWh', 'tooltip jumped to link'); |
There was a problem hiding this comment.
this expect got lost?
There was a problem hiding this comment.
There was a problem hiding this comment.
👍 sorry, missed that!
|
💃 once tests pass. Epic PR @etpinard 🏆 |
resolves #102
This PR builds on top of #1573 where annotation hover labels were added with support for custom bg color, border color and font settings. Trace hover labels will be customisable using the same attributes as for annotations, but unlike for annotations, these attributes will be
arrayOk: true. For example,Questions:
layout.?axis.hoverlabelattribute containers to make the the hover label that appear on the x(y) axes onhovermode: 'x'('y')customisable? UPDATE: commits f986fd2 and a8cc8b0 implementlayout.hoverlabelsrc/component/hover/? This new component would includegraph_interact.jswhich is becoming less and less a cartesian module. Done in Fx component #1613TODO:
heatmap/calc.jsto support 2D data arraysFx.hover(e.g. gl3d. gl2d) on hover