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.

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

Gauge resizing causes thresholds updates #1759

Closed
creage opened this issue Nov 3, 2020 · 4 comments
Closed

Gauge resizing causes thresholds updates #1759

creage opened this issue Nov 3, 2020 · 4 comments

Comments

@creage
Copy link

creage commented Nov 3, 2020

Description

Gauge Max value is changing during the resizing

Steps to check or reproduce

Go to https://naver.github.io/billboard.js/demo/#Chart.GaugeChart, paste in the code

var chart = bb.generate({
    data: {
        columns: [
            ["data", 500] // VALUE IS HIGHER THAN MAX
        ],
        type: "gauge"
    },
    gauge: {
        title: 'data',
        min: 0,
        max: 100 // MAX IS SET TO 100 ONLY!
    },
    color: {
        pattern: [
            "#FF0000",
            "#F97600",
            "#F6C600",
            "#60B044"
        ],
        threshold: {
            values: [
                30,
                60,
                90,
                100
            ]
        }
    },
    size: {
        height: 180
    },
    bindto: "#gaugeChart"
});

setTimeout(function () {
    chart.resize();
}, 2000);

Wait for 2 seconds, note the Max value of the gauge becomes 500, while the max is set to 100 only.

If you remove the title property from the gauge: {} - the value is always 500.

And I expect it to always be the gauge.max value - 100 in my example.

@michkami
Copy link
Collaborator

michkami commented Nov 3, 2020

As you can see below, there is a case, where config.gauge_max value can be overwritten with the totalSum.
If you trigger a resize, it will use the "new" gauge_max value for calculation and as max value.

const totalSum = $$.getTotalDataSum(state.rendered);
// if gauge_max less than totalSum, make totalSum to max value
if (totalSum > config.gauge_max) {
config.gauge_max = totalSum;
}
const gEnd = radius * (totalSum / (config.gauge_max - config.gauge_min));

Problem 1

It should be considered, if forcing the user to use the totalSum as gauge_max value is correct way to handle gauge_max.
In my opinion, config.gauge_max value should be prioritized over totalSum. If the user wants to use the total sum as max value, he can set it manually and should not be forced.

Problem 2

If you remove the title in the example above, it will show 100% as value, and 500 as max.
But through prior consideration gauge_max would be 100, and the vlaue in the middle would still show 100%, which is wrong.
If the value is greater than gauge_max it should be also considered in the calculation of the percentage and should not be cut off at 100%.

If this should be implemented/changed, it should also be implemented for gauge_type: multi, as maxValue can overwrite the config.gauge_max value.

const maxValue = $$.getMinMaxData().max[0].value;
// if gauge_max less than maxValue, make maxValue to max value
if (maxValue > config.gauge_max) {
config.gauge_max = maxValue;
}

@creage
Copy link
Author

creage commented Nov 3, 2020

@michkami super nice investigation, thanks a lot!

Personally I don't insist in either behavior, I just want it to be consistent. And resizing should not alter the data presented initially in the chart.

But I really like the idea of having more granular control over the min/max pair.

@netil netil added the bug label Nov 6, 2020
@netil
Copy link
Member

netil commented Nov 6, 2020

Thanks @creage, @michkami for the report & the investigation.
It seems gauge.max should be prioritized when option is set to make a consistent behavior.
I'll be digging on this.

@netil
Copy link
Member

netil commented Nov 6, 2020

Hm... after reviewing the code, prioritizing gauge.max isn't the solution.
The max value should be representing correct data value, where the text is the representation it.

So, if prioritize the text regardless the data value, is making incorrect representation of it.
Based on this criteria, for the example mentioned above, correct representation is 500.

In case of to make display some fixed text regardless the data amount, there's gauge.max.label.extent option.

    gauge: {
        min: 0,
	label: {
		extents: function(value, isMax) {
			return isMax ? 100 : value;  // will make display 100 for max
		},
	}
    },

netil added a commit to netil/billboard.js that referenced this issue Nov 6, 2020
Max label text should reflect data value, regardless the max option value set.

Ref naver#1759
@netil netil closed this as completed in 0c2006f Nov 6, 2020
netil pushed a commit that referenced this issue Nov 11, 2020
## [2.1.4](2.1.3...2.1.4) (2020-11-11)

### Bug Fixes

* **api:** fix tooltip showing when lesser data loaded ([74320cf](74320cf)), closes [#1761](#1761)
* **event:** fix referencing event element ([38568c1](38568c1)), closes [#1752](#1752)
* **gauge:** fix to be consistent max label value ([0c2006f](0c2006f)), closes [#1759](#1759)
* **radar:** fix labels showing on esm usage ([d56ff52](d56ff52)), closes [#1765](#1765)
* **shape:** shape not showing on ie11 ([d1366d1](d1366d1)), closes [#1758](#1758)
* **tooltip:** fix to reset pending events from .show() ([ce8210c](ce8210c)), closes [#1753](#1753)
* **zoom:** fix throwing TypeError during zoom  ([f2787fa](f2787fa)), closes [#1760](#1760)
@netil netil added the released label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants