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

fixed issue #19187 #19399

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fixed issue #19187 #19399

wants to merge 1 commit into from

Conversation

mertokten
Copy link

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

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.

Screenshot 2023-12-08 at 11 45 55 AM

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.

Screenshot 2023-12-08 at 11 48 04 AM

Document Info

One of the following should be checked.

  • This PR doesn't relate to document changes
  • The document should be updated later
  • The document changes have been made in apache/echarts-doc#xxx

Misc

ZRender Changes

  • This PR depends on ZRender changes (ecomfe/zrender#xxx).

Related test cases or examples to use the new APIs

N.A.

Others

Merging options

  • Please squash the commits into a single one when merging.

Other information

Copy link

echarts-bot bot commented Dec 13, 2023

Thanks for your contribution!
The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

// (https://github.com/apache/echarts/issues/19187)
if ((!maxFixed && minFixed) || (!minFixed && maxFixed)) {
if (min > max) {
max = min + 10;
Copy link
Contributor

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?

Copy link
Author

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!

Copy link
Contributor

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!

Copy link
Author

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!

Screenshot 2024-01-05 at 7 44 33 PM

@Ovilia
Copy link
Contributor

Ovilia commented Dec 14, 2023

Please do not use force pull in the future. This helps us better understand the change. Thanks! :)

Copy link
Contributor

The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-19399@c9131a5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants