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

Fix for AspectRatio configuration #88

Merged
merged 3 commits into from
Mar 27, 2020
Merged

Conversation

esso23
Copy link
Contributor

@esso23 esso23 commented Mar 27, 2020

Making it possible to not set width/height on chart canvas. This is required for AspectRatio to work.
I removed the default settings for width/height of 400/400 since it makes more sense to not have the canvas width/height set by default.

…equired for AspectRatio to work.

- As stated here: https://www.chartjs.org/docs/latest/general/responsive.html#important-note
"Note that this option is ignored if the height is explicitly defined either as attribute or via the style."
@Joelius300
Copy link
Contributor

Thank you for the fix! For future reference, you should first open an issue explaining the problem but you've done a great job at the PR description so you don't need to create one now :)

I will have to do some tests with it before merging but it looks like a straight forward fix.
One thing though. Could you remove the = null; assignments? Since the default of int? is always null, I dislike the explicit default value. This is just a small style thing but if we always do these things the same way, we might be able to some day get rid of all the style inconsistencies within the library.

Otherwise great work, I'm looking forward to incorporate this in the next release!

Copy link
Contributor

@Joelius300 Joelius300 left a comment

Choose a reason for hiding this comment

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

Have you compiled the project? As far as I know, this will raise a compilation error. I think you need to remove the trailing semicolon.

Semicolon after method or accessor block is not valid

@esso23
Copy link
Contributor Author

esso23 commented Mar 27, 2020

Damn, sorry for that. Did that before my morning coffee :X

Copy link
Contributor

@Joelius300 Joelius300 left a comment

Choose a reason for hiding this comment

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

No worries!

I don't have the possibility to test it right now but I'll merge it as soon as I've tested it. It should be fixed in the upcoming release.

Update: Tested and it works perfectly :)

@Joelius300 Joelius300 merged commit bc4cb3b into mariusmuntean:master Mar 27, 2020
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