`indicator`: several improvements/fixes #4029

antoinerg · 2019-07-08T14:36:07Z

This PR attempts to fix a few visual issues that can occur on animation:

fill color of delta is not consistent with the currently displayed value (ce3f2d1)
precompute the final size and position of numbers to rescale properly when the animation start (hence preventing overlap). We effectively save one Drawing.bBox call (1e7f21d)
numbers (either number or delta) can jump around if the text size changes a lot from the start value to end value. We solve this by aligning the numbers using the minimum X offset seen up to this point (2c378e0).
fix transition on first animation (6e9def2)

Additional bug fix:

it reverts to default valueformat for both number and delta if an empty string is specified (4d2ad95)
fix bullet with range spanning negative values (3b9028a)
do not display SI suffix when passing through zero if no SI suffixes are present in start and end value (8cea8e7)
add support for blank valueformat
do not draw value on gauge with null value
render exponent properly

https://codepen.io/antoinerg/pen/bPQEPr?editors=0010

src/traces/indicator/defaults.js

src/traces/indicator/plot.js

antoinerg · 2019-07-08T15:35:43Z

Do we have a mock testing the "range spanning negative values" case?

Added one in 68c2aef

antoinerg · 2019-07-11T19:58:03Z

Codepen has been updated to use bundle built off commit b942832: https://codepen.io/antoinerg/pen/bPQEPr?editors=0010

simplify opts passed in indicator plot

etpinard · 2019-07-11T21:05:37Z

@antoinerg are you planning on addressing

I would prefer if *tickformat: '' would result in the default Axes.tickText formatting

as mentioned in #4029 (comment) ?

Is Axes.tickText still giving you trouble? Maybe @archmoj can help?

That said, this is a small detail, I'd be ok punting this for later. The fixes here in this PR are fantastic!

antoinerg · 2019-07-11T21:07:58Z

Is Axes.tickText still giving you trouble? Maybe @archmoj can help?

It was giving me NaN and I didn't investigate much more. I still think using exponent or SI notation is a better default but consistency with the axes is also good... If @archmoj can get it to work I could get behind it.

archmoj · 2019-07-11T21:08:44Z

Is Axes.tickText still giving you trouble? Maybe @archmoj can help?

Yes the blank is not working.
I am on it.

antoinerg · 2019-07-11T21:16:37Z

Yes the blank is not working.
I am on it.

Thanks @archmoj ! Don't push directly on this branch as I will need to test it thoroughly before accepting the change.

archmoj · 2019-07-18T16:12:15Z

By defaulting tickformat to .3s we would get 1.00 and 10.0 as presented on the image on the left.
What is we could mock linear axis in a way that the tickText work with blank tickformat?
In that case we can get 1 and 10 as presented on the image on the right.

So I am wondering if the blank may be a better default?

First demo using modified version of '.3s' format - I noticed the m suffix may still show up sometimes.
Second demo using plolt.js Cartesian blank format - using different setup 1a05658

cc: @nicolaskruchten @alexcjohnson @jonmmease
Please also consider removing Math.floor from the JS code to compare transition between non-integers in two cases.

nicolaskruchten · 2019-07-18T17:29:30Z

Generally speaking, I prefer "blank as the default" rather than 3-decimals.

nicolaskruchten · 2019-07-18T17:30:10Z

But I don't think the default for negative values should be red + triangle-down + parens + minus-sign. that feels like overkill squared :)

antoinerg · 2019-07-18T18:15:12Z

But I don't think the default for negative values should be red + triangle-down + parens + minus-sign. that feels like overkill squared :)

@nicolaskruchten Which one should we ship as a default? A minus sign or parentheses?

nicolaskruchten · 2019-07-18T18:35:52Z

Minus sign, no parens plz

archmoj · 2019-07-18T20:28:47Z

What about default formatting for relative deltas?
It could default to either '2%' or blank. But which one is best?

etpinard · 2019-07-18T20:35:11Z

What about default formatting for relative deltas?
It could default to either '2%' or blank. But which one is best?

'2%' is probably fine here.

archmoj · 2019-07-18T20:36:55Z

What about default formatting for relative deltas?
It could default to either '2%' or blank. But which one is best?

'2%' is probably fine here.

Great! I also think so.

- use ax.setScale and Axes.calcTicks with mocked range [0, 1.5 * value] or the gauge.axis one to make that work. - fixup jasmine tests and baselines

... so that we don't get console warnings

etpinard · 2019-07-19T19:21:12Z

@antoinerg feel free to merge this PR 💃 for me!

antoinerg · 2019-07-19T19:51:14Z

Thank you @etpinard and @archmoj for helping me bring this to the finish line 🏁

antoinerg added 6 commits July 3, 2019 15:58

indicator: changes default delta.valueformat and delta alignment

2c378e0

use default valueformat if emptry string is provided

4d2ad95

set correct color for delta during animation, stash its last value

cb5d3c4

fix bullet with range spanning negative values

3b9028a

fix for delta.reference equal to zero

a8ccf2b

manually compute numbersbBox to rescale when the animation starts

1e7f21d

etpinard reviewed Jul 8, 2019

View reviewed changes

src/traces/indicator/defaults.js Outdated Show resolved Hide resolved

etpinard reviewed Jul 8, 2019

View reviewed changes

src/traces/indicator/plot.js Outdated Show resolved Hide resolved

etpinard reviewed Jul 8, 2019

View reviewed changes

src/traces/indicator/plot.js Outdated Show resolved Hide resolved

antoinerg added 2 commits July 8, 2019 11:27

lastValue: defaults to current value to fix first animation

6e9def2

add mock to test bullet spanning negative values

68c2aef

etpinard added this to the v1.49.0 milestone Jul 8, 2019

antoinerg and others added 3 commits July 11, 2019 15:28

animation: do not display SI prefix if not present in start and end

8cea8e7

indicator: remove + sign by default

b942832

simplify passing opts in indicator plot.js

5590398

archmoj and others added 2 commits July 11, 2019 16:01

use axisLayer for clarity

438c89c

Merge pull request #4037 from plotly/indicator-fixes_moj_simplify-opts

fc5c2c9

simplify opts passed in indicator plot

etpinard added status: reviewable bug labels Jul 11, 2019

archmoj and others added 5 commits July 19, 2019 15:09

add new mocks to test all attrributes and number formatting

0e036a0

use '' valueformat as dflt

bd0aa79

- use ax.setScale and Axes.calcTicks with mocked range [0, 1.5 * value] or the gauge.axis one to make that work. - fixup jasmine tests and baselines

show NaN values as - + use convertToTspans to render exponents ok

3acb333

draw blank value arc/bullet when value is NaN

0160e16

... so that we don't get console warnings

Merge branch 'master' into indicator-fixes

38be82d

antoinerg merged commit 72f885a into master Jul 19, 2019

antoinerg deleted the indicator-fixes branch July 19, 2019 19:51

Mahdis-z mentioned this pull request Jul 25, 2019

indicator trace type plotly/documentation#1411

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GitHub Sponsors

`indicator`: several improvements/fixes #4029

`indicator`: several improvements/fixes #4029

antoinerg commented Jul 8, 2019 •

edited

Loading

antoinerg commented Jul 8, 2019 •

edited

Loading

antoinerg commented Jul 11, 2019 •

edited

Loading

etpinard commented Jul 11, 2019

antoinerg commented Jul 11, 2019

archmoj commented Jul 11, 2019

antoinerg commented Jul 11, 2019

archmoj commented Jul 18, 2019

nicolaskruchten commented Jul 18, 2019

nicolaskruchten commented Jul 18, 2019

antoinerg commented Jul 18, 2019

nicolaskruchten commented Jul 18, 2019

archmoj commented Jul 18, 2019

etpinard commented Jul 18, 2019

archmoj commented Jul 18, 2019

etpinard commented Jul 19, 2019

antoinerg commented Jul 19, 2019

indicator: several improvements/fixes #4029

indicator: several improvements/fixes #4029

Conversation

antoinerg commented Jul 8, 2019 • edited Loading

antoinerg commented Jul 8, 2019 • edited Loading

antoinerg commented Jul 11, 2019 • edited Loading

etpinard commented Jul 11, 2019

antoinerg commented Jul 11, 2019

archmoj commented Jul 11, 2019

antoinerg commented Jul 11, 2019

archmoj commented Jul 18, 2019

nicolaskruchten commented Jul 18, 2019

nicolaskruchten commented Jul 18, 2019

antoinerg commented Jul 18, 2019

nicolaskruchten commented Jul 18, 2019

archmoj commented Jul 18, 2019

etpinard commented Jul 18, 2019

archmoj commented Jul 18, 2019

etpinard commented Jul 19, 2019

antoinerg commented Jul 19, 2019

`indicator`: several improvements/fixes #4029

`indicator`: several improvements/fixes #4029

antoinerg commented Jul 8, 2019 •

edited

Loading

antoinerg commented Jul 8, 2019 •

edited

Loading

antoinerg commented Jul 11, 2019 •

edited

Loading