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

Default height no longer working in 2.8 #176

Closed
adrianlungu opened this issue Aug 21, 2017 · 7 comments · Fixed by #187
Closed

Default height no longer working in 2.8 #176

adrianlungu opened this issue Aug 21, 2017 · 7 comments · Fixed by #187
Assignees
Milestone

Comments

@adrianlungu
Copy link

Expected Behavior

Charts to be rendered even if the chart is inside an element which has display:none set.

Actual Behavior

Charts not being rendered unless the element they are inside is visible at the moment the data is being set

This is working fine in 2.7.2 but in 2.8.2 it seems the default height of the component is not being set anymore and the canvas has 0 height unless it is visible when the data is being set. I've checked with 2.8.1 and 2.8 and they're both behaving the same, 2.7.2 is working fine though.

Environment

  • vue.js version: 2.4.2
  • vue-chart.js version: 2.8.2
  • npm version: 5.3.0
@apertureless
Copy link
Owner

Can you post a minimal codepen / jsfiddle for reproduction?

3oEdva9BUHPIs2SkGk

@adrianlungu
Copy link
Author

adrianlungu commented Aug 25, 2017

Is a webpackbin ok ?

2.8.2 - Not working
https://www.webpackbin.com/bins/-KsOvI4Af8rgy1cYgA-H

2.7.2 - Working
https://www.webpackbin.com/bins/-KsOvXb81hsKAh3S-zN4

Thanks!

@adrianlungu
Copy link
Author

On a closer look while making the repos, it seems the cause is this

maintainAspectRatio: false

In 2.7.2, when changing from display: none to anything else, the chart still renders, in 2.8.2, unless I manually edit the height of the chart (simply disabling 100% on height on the chart div), it doesn't show up.

I'm not sure if I'm using it wrong or that's how it's supposed to work though.

@apertureless
Copy link
Owner

With the 2.8.x release I've added dynamic inline styles to the outer container.
It seems the width: 100%; height: 100%; position: relative; which gets applied as inline styles are causing this. 🤔

Adding min-height: 100% seems to fix this. I will investigate further.

However a quick fix for you would be to pass :styles="{}" to your chart until I release a fix. So it will overwrite the default styles.

@adrianlungu
Copy link
Author

adrianlungu commented Aug 25, 2017

The quick fix works great, thanks!

I've added an empty styles object as a prop to the chart object I'm extending to avoid adding the tag to every chart in the project though. Seems to work as expected!

@apertureless apertureless self-assigned this Aug 28, 2017
@apertureless apertureless added this to the 2.8.3 milestone Sep 2, 2017
@apertureless
Copy link
Owner

Ok, figured out the problem.

You set a class with a height: 200px to the div. And the inline-styles were overwriting it to height: 100%.

But, as you were hiding your component with display: none and not v-if the chart got rendered, checked the outer div, which got a height of 0 because the display: none and its own height was 100% so it was set to 0.

I removed the default styles as they seem to cause some trouble. So it should be fixed in the next version 2.8.3.

btw. you should consider using v-if is it will unmount and destroy the component, not only hide.

If you have 200 charts but alle hidden with display: none all of them will mount and render. With v-if they will only be mounted if its true .

apertureless added a commit that referenced this issue Sep 2, 2017
This will fix #176

Signed-off-by: Jakub Juszczak <netghost03@gmail.com>
@adrianlungu
Copy link
Author

Thanks, I'll update to the next version once it's out!

I ran into this while using the Tabs component from UIV, it seems to be using v-show internally so I might look into maybe making a request to change it to v-if, although, there are certain advantages to v-show vs v-if under specific conditions.

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

Successfully merging a pull request may close this issue.

2 participants