-
Notifications
You must be signed in to change notification settings - Fork 19.6k
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
fixed issue #19187 #19399
base: master
Are you sure you want to change the base?
fixed issue #19187 #19399
Conversation
Thanks for your contribution! |
// (https://github.com/apache/echarts/issues/19187) | ||
if ((!maxFixed && minFixed) || (!minFixed && maxFixed)) { | ||
if (min > max) { | ||
max = min + 10; |
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.
What does the magic number 10 mean 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.
@Ovilia, thank you for your response.
My reasoning behind the 10 offset was since the user won't supposed to see any data on the graph with a min that does not get override, the user should still see a range of numbers for the labels.
However, I can change the set magical number logic or set it to something else. I would be happy if I get your further feedback on the topic. Thanks!
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.
Typically, we provide an option like 'xAxis.xxx' for this value. Thanks!
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.
If I remove the magic number, y axis range becomes 2.5 as min and 5 as max as in the example below. With this, it does not show the data that is not in the ymin range as it should, however, it still ignores the min threshold that the user set (ymin = 5 in this example below).
Would removing the magic number fix the issue or what do you suggest?
Thanks!
Please do not use force pull in the future. This helps us better understand the change. Thanks! :) |
The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-19399@c9131a5 |
Brief Information
This pull request is in the type of:
What does this PR do?
It fixes the bug occuring in test/min-max-axis.html test case.
Fixed issues
Details
Before: What was the problem?
The bar-chart ignores the yAxis.min setting and instead seems to use the auto-calculated value. In min-max-axis.html test case as you can see, the max is not set so it fails auto-calculating the axis labels.
After: How does it behave after the fixing?
The yAxis minimum value should be as specified. (yAxis min is set 5 in our case). It was fixed by when either one is fixed and the other is not,by making sure that the min is always smaller than the max and also the vice versa. Finally after adjusting the min and max we set the minFixed or maxFixed to true so that it won't auto calculate a wrong axis label.
Document Info
One of the following should be checked.
Misc
ZRender Changes
Related test cases or examples to use the new APIs
N.A.
Others
Merging options
Other information