-
-
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
fixup hoverinfo & hovertemplate initial, delta and final flags for waterfalls #3963
Conversation
… in waterfall similar to PR 3958
This is cool, but there are some differences between the final text in the on-plot text and the in-hover text:
@etpinard what's the right way forward here? |
Good call. |
Oh right, we could have done a better job explaining the That said, (1) nothing guarantees than Come to think of it, the currently behavior isn't that bad as I doubt that most users will use a |
src/traces/waterfall/hover.js
Outdated
} | ||
if(hasFlag('delta') && point.deltaLabel !== '') { | ||
if(size < 0) { | ||
text.push('(' + point.deltaLabel + ') ' + DIRSYMBOL.decreasing); |
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.
this here should be the deltaLabel
value so that
hoverinfo: 'delta'
and
hovertemplate: '%{delta}'
render the same.
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.
As for Final:
and Initial:
prefixes, they should not be part of finalLabel
/ initalLabel
.
See example:
https://codepen.io/etpinard/pen/ewJNVJ
In fact, those prefixes should only be there when there are more than 1 of 'initial', 'delta' and 'final' flags.
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.
In brief, xLabel
should have the default-formatted label text for x
.
I'd argue that the (v)
around negative deltas should be considered formatting, but the DIRSMBOL
suffixes should not.
So in brief,
v = formatNumber(Math.abs(point.delta));
point.deltaLabel = size < 0 ? '(' + v + ')' : v;
// ...
if(size < 0) {
text.push(point.deltaLabel + DIRSYMBOL.decreasing))
} else {
text.push(point.deltaLabel + DIRSYMBOL.increasing);
}
``
test/jasmine/tests/waterfall_test.js
Outdated
.then(done); | ||
}); | ||
|
||
it('should turn on percentages with hoveinfo all', function(done) { |
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.
Looks like a <ctrl-c><ctrl-v>
gone bad, there are no percentages here 😏
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.
Good eye! It was 2/3am.
test/jasmine/tests/waterfall_test.js
Outdated
@@ -1424,7 +1486,7 @@ describe('waterfall hover', function() { | |||
}) | |||
.then(function() { | |||
assertHoverLabelContent({ | |||
nums: '2.2\n4.4 ▲\nInitial: −2.2', | |||
nums: '2.2\nFinal: 2.2\n4.4 ▲\nInitial: −2.2', |
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.
Hmm. So wait Final:
is new?
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.
Yes.
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.
Was this a requirement anywhere?
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.
No. And we could possibly drop that.
But that make it clear what the value is pointing to.
test/jasmine/tests/waterfall_test.js
Outdated
@@ -1460,31 +1522,31 @@ describe('waterfall hover', function() { | |||
.then(function() { | |||
var out = _hover(gd, 0, 1000.5, 'closest'); | |||
expect(out.yLabelVal).toEqual(1002.201); | |||
expect(out.extraText).toEqual(undefined); | |||
expect(out.extraText).toEqual('Final: $1,002.201m<br>$2.2m ▲<br>Initial: $1,000.001m'); |
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 did these expectations change?
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 did not fill any extra text for blue bars (totals). But it is pretty useful to display this info for them for example when there is a base
other than zero.
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, but what does initial and final refer to for total bars?
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.
The base and top of the bar.
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 don't think base/top here represent inital and final values.
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 be displayed in codepen after.
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 agree that initial and final don't make sense in 'total' cases
This is ready for the second review. |
src/traces/waterfall/hover.js
Outdated
if(di.isSum) { | ||
point.final = undefined; | ||
point.initial = undefined; | ||
point.delta = size - di.b; |
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.
Hmm. I'm not sure we can call this a "delta" . point.y
is probably best for the total bars.
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.
In respect to #3963 (comment) displaying delta seems to be useful especially when there is a base; also when hovering using hoverinfo
: delta
.
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.
To make it similar to current behaviour, this option is dropped in 8432d8f.
test/jasmine/tests/waterfall_test.js
Outdated
@@ -1424,7 +1486,7 @@ describe('waterfall hover', function() { | |||
}) | |||
.then(function() { | |||
assertHoverLabelContent({ | |||
nums: '2.2\n4.4 ▲\nInitial: −2.2', | |||
nums: '2.2\n2.2\n4.4 ▲\nInitial: −2.2', |
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 do we have an extra 2.2
now?
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.
In respect to #3963 (comment) displaying delta seems to be useful especially when there is a base; also when hovering using hoverinfo
: delta
.
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.
The extra 2.2 here is the final
value which is the same as y
.
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.
Addressed in 0a58836.
Very nice implementation 💃 |
A follow up of #3954, #3958 and to address #3960.
The
initial
,delta
andfinal
displayed in thewaterfall
hover now could be controlled and turned off usinghovertemplate
andhoverinfo
.Also to note for consistency the initial values and delta from
base
is also displayed over the total bars.Codepen before
Codepen after
@plotly/plotly_js
cc: @nicolaskruchten