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

Doughnut scriptable options #5966

Merged
merged 6 commits into from Jan 10, 2019

Conversation

etimberg
Copy link
Member

@etimberg etimberg commented Jan 8, 2019

Background

This PR introduces scriptable configuration options for doughnut charts. The user can script the styling, interaction, and border alignment options.

Testing

Unit tests were written to cover these features and were based on the existing bubble controller tests. The existing unit tests covering the setHoverStyle dataset controller method were removed as they did not have proper setup for use with scriptable options.

Documentation

I updated the doughnut chart documentation to match the style of the bubble & bar charts as they already support scriptable options.

nagix
nagix previously requested changes Jan 8, 2019
docs/charts/doughnut.md Outdated Show resolved Hide resolved
docs/charts/doughnut.md Outdated Show resolved Hide resolved
@nagix
Copy link
Contributor

nagix commented Jan 8, 2019

Looks good to me except for a few minor errors. Are you going to add fixture tests and a sample?

@etimberg
Copy link
Member Author

etimberg commented Jan 8, 2019

I will add fixture tests but I don't have anytime today so I can't get to it until tomorrow. I'll see what we have for the bar & bubble charts in terms of this. The interaction tests should cover a fair bit of it already.

@simonbrunel
Copy link
Member

Looks good, I agree with @nagix that we should also add image based unit test, similar to these ones.

@etimberg
Copy link
Member Author

etimberg commented Jan 9, 2019

@simonbrunel @nagix I added some scriptable fixture tests for backgroundColor, borderColor, and borderWidth. I probably should write one test for borderAlign and then this should be good to go

@etimberg
Copy link
Member Author

etimberg commented Jan 9, 2019

I've added tests for borderAlign so this should be good to go.

@etimberg etimberg dismissed nagix’s stale review January 9, 2019 23:52

Addressed concerns regarding missing tests

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.

Looks very good, except for the duplication of couple of lines, reported by codeclimate.
We'll see how it turns out when more scriptable options are implemented and refactor later if needed.

@simonbrunel simonbrunel merged commit e1ed26f into chartjs:master Jan 10, 2019
@etimberg etimberg deleted the doughnut-scriptable-options branch October 27, 2019 20:46
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.

None yet

4 participants