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

Upgrade to chartjs v4 and fix bugs #170

Merged
merged 12 commits into from
Mar 23, 2024
Merged

Upgrade to chartjs v4 and fix bugs #170

merged 12 commits into from
Mar 23, 2024

Conversation

santam85
Copy link
Collaborator

Thanks for contributing.

Description

Upgraded the library to work with Chart.js v4. Change of configuration APIs was needed in order to avoid Chart.js errors.
Improved the testing examples in docs to showcase improved animations, color defaults matching the default Chart.js theme, updated code style, linting and packaging to match Chart.js standards.

Testing

Check docs/index.html

Copy link
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

there's really no way to review this with code changes and formatting changes mixed together. it probably would have been better to do the reformatting as a separate PR from the actual changes

# Replace stock eslint rules with typescript-eslint equivalents for proper
# TypeScript support.
indent: "off"
"@typescript-eslint/indent": ["error", 2]
Copy link
Collaborator

Choose a reason for hiding this comment

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

there's a mix of formatting in this file. this rule is surrounded by double quotes and it's single quotes two lines below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

docs/index.html Outdated Show resolved Hide resolved
docs/index.html Outdated Show resolved Hide resolved
src/controller.financial.js Outdated Show resolved Hide resolved
@santam85
Copy link
Collaborator Author

there's really no way to review this with code changes and formatting changes mixed together. it probably would have been better to do the reformatting as a separate PR from the actual changes

agreed, I jumped the gun a bit here, in the spirit of expediting alignment with the parent project.

@santam85 santam85 requested a review from benmccann March 13, 2024 09:10
@benmccann
Copy link
Collaborator

I'll leave it up to you then. Go ahead and merge if you feel confident enough in it. I don't think there's much help I can provide from looking at the diff

@santam85
Copy link
Collaborator Author

santam85 commented Mar 14, 2024

Sounds good, will add a bit more information about the changes in the README.md and then merge/release.

@santam85 santam85 merged commit e898295 into master Mar 23, 2024
3 checks passed
@santam85 santam85 deleted the major/chartjs-4 branch March 23, 2024 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants