Skip to content

Responsive property doesn't work #539

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

Closed
Karpengold opened this issue Aug 2, 2019 · 8 comments
Closed

Responsive property doesn't work #539

Karpengold opened this issue Aug 2, 2019 · 8 comments

Comments

@Karpengold
Copy link

I used Line chart with options:

 responsive: true,
 maintainAspectRatio: false,

And use wrappers for chart:

<div class="main">

    <div class="chart">
      <chart
        :chart-data="datacollection"
        :options="chartoptions"
      />
    </div>

  </div>


.chart {
  max-width: 80%;
  max-height: 30%;
  position: relative;
}


.main {
  height: 250px;
  width: 250px;
  position: absolute;
}

but chart doesn't scale by height

image

@tombraul
Copy link

I can confirm this.

I am using vue 2.6.10, vue-chartjs 3.4.2 and chart.js 2.8.0

@apertureless
Copy link
Owner

Can you maybe provide a codepen or other runable reproduction?

@dan24678
Copy link

dan24678 commented Dec 9, 2019

@apertureless -- I believe I have found the cause of this (or at least a related issue) and have a suggestion for a fix.

Chart.js docs when describing 'aspectRatio': "Note that this option is ignored if the height is explicitly defined either as attribute or via the style." per https://www.chartjs.org/docs/latest/general/responsive.html#configuration-options

vue-chartjs does not offer any facility to NOT define the height and width. They always default to being 400x400 inlined in the html, even when they are not defined, which means the "aspectRatio" property (and responsive design in general?) will simply never work.

This is the problematic code: https://github.com/apertureless/vue-chartjs/blob/develop/src/BaseCharts.js#L16

I have verified that the following modification to BaseChart.js enables a responsive design and aspectRatio to function as intended:

node-app____src_node-app__-_____ui_node_modules_vue-chartjs_es_BaseCharts_js

The reproduction case would basically be the inability of the current code to utilize the aspectRatio property. When you make ANY chart with the following configuration you should see a chart that is 4 times as wide as it is tall. The chart should expand to fill the full width of the browser:

chartOptions: { responsive: true, aspectRatio: 4, maintainAspectRatio: true, },

Conversely, this code should make a chart that is 4 times as tall as it is wide:

chartOptions: { responsive: true, aspectRatio: 0.25, maintainAspectRatio: true, },

With the current version of your code, this functionality is only partially working. The chart does expand to fill the full width of the browser, but it does not respect the aspectRatio property at all and always has a 1:1 height/width ratio.

Fortunately, because the observed default behavior of not specifying a height and width is the same between both versions of the code (ie, they both result in 100% width, and not 400px width), I wouldn't consider this bug fix a breaking change and am hoping you can get it deployed to npm soon, if you are able and agree with the proposed solution.

I'm happy to send it through as a PR if that might expedite things. Thanks.

@dan24678
Copy link

dan24678 commented Dec 9, 2019

Yay! I found a workaround with the current code. If you pass in null for height and width, it will respect those values and aspectRatio will still work. I do still think this is a pretty critical bug, but at least I don't need to wait on a fix now.

Here's what this looks like:

node-app____src_node-app__-_____ui_src_components_pages_CoachHistory_vue

@apertureless
Copy link
Owner

Ah nice finding!
Yeah we have already a PR for this: #504

However, removing the defaults is a breaking change. Thus I wanted to rewrite the reactive mixins, before merging this.

@ImSeaWorld
Copy link

@apertureless Any updates on that? No movement in a while.
@DrLongGhost Thank you good sir! Works perfectly!

@apertureless apertureless added this to the 4.0.0 milestone Aug 28, 2020
@apertureless
Copy link
Owner

Not much time currently.
However I will most likeley release it together with #513 #503 #601 as version 4.0.0
Because then I have all breaking changes in one release. Makes it easier to migrate.

@ThiagoMatosBR
Copy link

@DrLongGhost you are a life saver!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants