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

feat(bar): add startValue option to specify where the bars start #17078

Merged
merged 6 commits into from
May 21, 2024

Conversation

jiawulin001
Copy link
Member

@jiawulin001 jiawulin001 commented May 21, 2022

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • others

What does this PR do?

barSeries.startValue is added for users to specify where the bars start.

Fixed issues

Details

Before: What was the problem?

As stated in the issues. Users cannot specify the start value of bars.

issue #16840 issue #17077
image image

After: How is it fixed in this PR?

Users now can specify barSeries.startValue to set the starting value of bars. Stack and multi-axes are also tested to be running correctly.

issue #16840 (startValue = -200) issue #17077 (startValue = -150)
after #16840 after #17077

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@670e4a2

Misc

ZRender Changes

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

Related test cases or examples to use the new APIs

bar-startValue.html

Others

Merging options

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

Other information

@echarts-bot
Copy link

echarts-bot bot commented May 21, 2022

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.

Document changes are required in this PR. Please also make a PR to apache/echarts-doc for document changes and update the issue id in the PR description. When the doc PR is merged, the maintainers will remove the PR: awaiting doc label.

@echarts-bot echarts-bot bot added PR: awaiting doc Document changes is required for this PR. PR: awaiting review labels May 21, 2022
@susiwen8
Copy link
Contributor

susiwen8 commented May 23, 2022

@jiawulin001
Copy link
Member Author

Sure, it's done.

@Ovilia
Copy link
Contributor

Ovilia commented May 29, 2022

This looks to be a useful feature, only that more test cases are required. For example, to test the cases when min or max values are defined for the axis, or when r does start from 0 for polar bars.

And you need to run the visual tests to test the differences caused by this PR.

npm run test:visual

Choose the latest nightly build to compare with the one build from your source code.

@jiawulin001 jiawulin001 dismissed a stale review via 7eae814 June 13, 2022 07:13
@jiawulin001
Copy link
Member Author

@Ovilia Extra testcases are added and all visual tests are passed.

src/layout/barGrid.ts Outdated Show resolved Hide resolved
test/bar-startValue.html Outdated Show resolved Hide resolved
src/layout/barGrid.ts Outdated Show resolved Hide resolved
@Ovilia Ovilia changed the base branch from master to next June 13, 2022 07:31
@Ovilia Ovilia changed the base branch from next to master June 13, 2022 07:32
@Ovilia
Copy link
Contributor

Ovilia commented Jun 13, 2022

It seems there are a lot of commits that are already in the master branch. Try this solution.

@Ovilia
Copy link
Contributor

Ovilia commented Jun 13, 2022

Please also add a startValue test with dataZoom component.

@susiwen8 susiwen8 added this to the 5.4 milestone Jun 13, 2022
Now the changes are more naturally co-op with original code and can be applied in dataZoom.
@pull-request-size pull-request-size bot added size/M and removed size/S labels Jun 14, 2022
@jiawulin001
Copy link
Member Author

jiawulin001 commented Jun 14, 2022

@olivia Good intuition on dataZoom! I tested and found startValue not compatible with dataZoom.
I've revised the code and now it works well with dataZoom and more naturally incorporate in original code. Also min/max set by user now has higher priority over startValue. Another change is that startValue is moved under axis rather than series, which I think is more reasonable.
Relevant test cases concerning dataZoom, priority and polar are all added. Please check them out.

@dailijian
Copy link

@jiawulin001 Merging is blocked,

@Ovilia Ovilia modified the milestones: 5.4, 5.4.1 Sep 1, 2022
@plainheart plainheart modified the milestones: 5.4.1, 5.5.0 Nov 10, 2022
@plainheart plainheart modified the milestones: 5.5.0, TBD Jun 15, 2023
@Ovilia Ovilia modified the milestones: TBD, 5.5.1 May 21, 2024
Copy link

echarts-bot bot commented May 21, 2024

To reviewers: If this PR is going to be described in the changelog in the future release, please make sure this PR has one of the following labels: PR: doc ready, PR: awaiting doc, PR: doc unchanged

This message is shown because the PR description doesn't contain the document related template.

@Ovilia Ovilia merged commit a4db0d4 into apache:master May 21, 2024
Copy link

echarts-bot bot commented May 21, 2024

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

@plainheart plainheart changed the title feat: barSeries.startValue added feat(bar): add startValue option to specify where the bars start Jun 18, 2024
@@ -81,6 +81,8 @@ export interface BarSeriesOption

showBackground?: boolean

startValue?: number
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option is unused here and should be removed in the next version.

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