Add aspectRatio property to responsive doc #5756
Conversation
docs/general/responsive.md
Outdated
| @@ -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`. | |||
There was a problem hiding this comment.
I would replace Integer by Number since it can be 0.5
There was a problem hiding this comment.
Sure, give a sec and I'll update it.
|
@simonbrunel @etimberg PR updated with the last comments. |
docs/general/responsive.md
Outdated
| @@ -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`. | |||
There was a problem hiding this comment.
Do we need the parens around width / height?
There was a problem hiding this comment.
We can remove it, I'll push the fix in a sec
|
@benmccann PR updated with last feedback |
docs/general/responsive.md
Outdated
| @@ -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`. | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Actually, I would try to keep the description impersonal (i.e. remove "you"). What about:
Canvas aspect ratio (i.e.
width / height, a value of1representing 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
|
I like making it impersonal as well. Simon's suggestion sounds good to me |
|
@benmccann @simonbrunel Cool, I'll make the change during the day |
|
Updated |
|
Thanks @danielcb29 |
Hi There!
This PR includes a single but really useful property documentation for custom aspect ratio charts.
What was my motivation behind this PR?:
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:
Thanks! 馃殌