Skip to content

feat: add borderRadius to bar charts. Closes #7701 #7951

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

Merged
merged 8 commits into from
Oct 26, 2020

Conversation

danmana
Copy link
Contributor

@danmana danmana commented Oct 24, 2020

Implemented border-radius for bar charts. Closes #7701
image

Support for stacked bar charts is not something trivial to add, so it's not included in this PR.
I tried looking into it, but we depend on borderSkipped for the implementation, and it doesn't work as expected for stacked bar charts: it skips a border in each rectangle from a stack, but we would need it to skip two borders for the middle rectangles and one border for the bottom of the stack.

One of the visual tests was failing due to a small, imperceptible, antialiasing difference between ctx.fillRect() and ctx.rect(); ctx.fill() (tested on Chrome). The change was done for code consistency between rounded and non-rounded rectangles.
I updated the fixture to match this.
Actual:
horizontal-borders-actual
Expected:
horizontal-borders-expected
Diff:
The difference was found only in two places, so it's not something consistent, but probably caused by anitaliasing. For reference the diff is 1 px tall and has colors: #cbcbcb and #cacaca
horizontal-borders-diff

@etimberg etimberg requested a review from kurkle October 24, 2020 19:26
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Thanks for the very thorough PR! It's great to see that you included the Typescript changes and everything.

Would it be possible to add a few image based tests of this behaviour? That way we can be sure that it doesn't break in the future

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Well done!

@danmana
Copy link
Contributor Author

danmana commented Oct 26, 2020

Would it be possible to add a few image based tests of this behaviour?

Sure 👍

@etimberg etimberg merged commit 495c359 into chartjs:master Oct 26, 2020
@danmana danmana deleted the border-radius branch October 26, 2020 15:15
@levipadre
Copy link

Hi,
I would love this feature. When can I expect it?

@danmana
Copy link
Contributor Author

danmana commented Nov 5, 2020

When can I expect it?

It should already be available in 3.0.0-beta5 - see sample and docs

@levipadre
Copy link

Thanks, great! Can you estimate release date?

@danmana
Copy link
Contributor Author

danmana commented Nov 5, 2020

It is already released. npm i -s [email protected] https://www.npmjs.com/package/chart.js/v/3.0.0-beta.5

@levipadre
Copy link

Sorry, I meant officially. When will it become the latest? Roughly?

@danmana
Copy link
Contributor Author

danmana commented Nov 5, 2020

I have no idea when 3.0 will go out of beta, I'm not one of the chart.js developers, I just made this PR.
You should follow #6598

@levipadre
Copy link

Sorry then, but thanks for the help

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.

Bar Chart Border Radius
4 participants