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

Added note about maintainAspectRatio #5974

Merged
merged 2 commits into from
Jan 13, 2019

Conversation

janelledement
Copy link
Contributor

Hi there,
This is rather a small documentation request, but do you think it might be worth it to explicitly say in the responsive.md file that in order for the given example to work, maintainAspectRatio property must be set to false? Here is a screenshot of the part I'm referring to:
chart_js_responsive_md_at_master_ _chartjs_chart_js
I realize that in the example codepen you provide, the said property is set to false. However, when I created my first chart, I just ran what I saw on the page above, then scratched my head for a while when it didn't work as I expected it to. I ended up comparing my code with the codepen example and eventually figured it out.

I'm thinking that by making a small note about the property on the documentation page might cost someone less mental overhead and time in the future.
online_markdown_editor_-_dillinger__the_last_markdown_editor_ever_

Thanks for your consideration!

@simonbrunel
Copy link
Member

Thanks @janelledement.

I would have expected the chart to resize to fit in its parent (something like CSS background-size: contain) but that's not the case when maintainAspectRatio: true, so I understand your confusion.

Your statement only applies to modifying the height, so I would update the example to also include changing the width:

chart.canvas.parentNode.style.height = '128px';
chart.canvas.parentNode.style.width= '128px';

Then modify your note to be something like:

Note that in order for the above code to correctly resize the chart height, the [`maintainAspectRatio`](#configuration-options) option must be set to `false`.

@janelledement
Copy link
Contributor Author

@simonbrunel Yes, I agree, thank you for the response! Change made

@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 13, 2019
@simonbrunel simonbrunel merged commit 352e53a into chartjs:master Jan 13, 2019
@simonbrunel
Copy link
Member

Thanks @janelledement

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.

4 participants