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

Add aspectRatio property to responsive doc #5756

Merged
merged 5 commits into from
Oct 9, 2018

Conversation

danielcb29
Copy link
Contributor

Hi There!

This PR includes a single but really useful property documentation for custom aspect ratio charts.

What was my motivation behind this PR?:

  • The tons of time I spent looking into different issues, StackOverflow pages, and blogs about how to put a 1:1 aspect ratio to my chart.
  • A lot of comments on different issues where people ask to add this property to the docs
  • The powerful feature that this property has.

This property is currently fully supported and tested, you just forgot to add it into the docs.

Please review if you feel comfortable with the description I added. If you want to propose another one just comment and I'll gladly change it :).

I hope this change can help a lot of people.

You can find a preview of the MD file in the following screenshot:
aspectratiopreview

Thanks! 🚀

etimberg
etimberg previously approved these changes Oct 4, 2018
@@ -16,6 +16,7 @@ Chart.js provides a [few options](#configuration-options) to enable responsivene
| `responsive` | `Boolean` | `true` | Resizes the chart canvas when its container does ([important note...](#important-note)).
| `responsiveAnimationDuration` | `Number` | `0` | Duration in milliseconds it takes to animate to new size after a resize event.
| `maintainAspectRatio` | `Boolean` | `true` | Maintain the original canvas aspect ratio `(width / height)` when resizing.
| `aspectRatio` | `Integer` | `(width / height)` | You can define your custom aspect ratio. If you want a 1:1 aspect ratio just set this property to 1. This doesn't work when you define a custom `width` and `height`.
Copy link
Member

Choose a reason for hiding this comment

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

I would replace Integer by Number since it can be 0.5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, give a sec and I'll update it.

@danielcb29
Copy link
Contributor Author

@simonbrunel @etimberg PR updated with the last comments.

@@ -16,6 +16,7 @@ Chart.js provides a [few options](#configuration-options) to enable responsivene
| `responsive` | `Boolean` | `true` | Resizes the chart canvas when its container does ([important note...](#important-note)).
| `responsiveAnimationDuration` | `Number` | `0` | Duration in milliseconds it takes to animate to new size after a resize event.
| `maintainAspectRatio` | `Boolean` | `true` | Maintain the original canvas aspect ratio `(width / height)` when resizing.
| `aspectRatio` | `Number` | `(width / height)` | You can define your custom aspect ratio. If you want a 1:1 aspect ratio just set this property to 1. This doesn't work when you define a custom `width` and `height`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the parens around width / height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove it, I'll push the fix in a sec

@danielcb29
Copy link
Contributor Author

@benmccann PR updated with last feedback

etimberg
etimberg previously approved these changes Oct 6, 2018
@@ -16,6 +16,7 @@ Chart.js provides a [few options](#configuration-options) to enable responsivene
| `responsive` | `Boolean` | `true` | Resizes the chart canvas when its container does ([important note...](#important-note)).
| `responsiveAnimationDuration` | `Number` | `0` | Duration in milliseconds it takes to animate to new size after a resize event.
| `maintainAspectRatio` | `Boolean` | `true` | Maintain the original canvas aspect ratio `(width / height)` when resizing.
| `aspectRatio` | `Number` | `width / height` | You can define your custom aspect ratio. If you want a 1:1 aspect ratio just set this property to 1. This doesn't work when you define a custom `width` and `height`.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean "This doesn't work" ? Is it that "This setting doesn't take effect" ? Or does something break and cause an error in the application?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means the first thing: "This setting doesn't take effect". If you see platform.don.test.js file, test: should not apply aspect ratio when height specified you can confirm that the property doesn't take any effect when you apply a specific height. I can update this description with the text you proposed to be more clear on what it means. Thanks

Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

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

Actually, I would try to keep the description impersonal (i.e. remove "you"). What about:

Canvas aspect ratio (i.e. width / height, a value of 1 representing a square canvas). Note that this option is ignored if the height is explicitly defined either as attribute or via the style.

@benmccann what do you think?

Also, the default value is 2

@benmccann
Copy link
Contributor

I like making it impersonal as well. Simon's suggestion sounds good to me

@danielcb29
Copy link
Contributor Author

@benmccann @simonbrunel Cool, I'll make the change during the day

@danielcb29
Copy link
Contributor Author

Updated

@simonbrunel simonbrunel merged commit 1ba06a2 into chartjs:master Oct 9, 2018
@simonbrunel
Copy link
Member

Thanks @danielcb29

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