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

avoid not needed updates for the chart, when dom element changes #52

Closed
wants to merge 6 commits into from
Closed

Conversation

dirk-ecker
Copy link

When using animation like in this example

<pie-chart :title="'Anteile an den Gesamtausgaben'" :data="openAccessBarometer.costsPublisher" thousands="." decimal="," legend="right" :suffix="openAccessBarometer.relative ? '%' : '€'" :library="{animation: {duration: 1000}}" download="true" :messages="{empty: 'Keine Daten verfügbar.'}"></pie-chart>

you will see that every change in the dom creates a repaint of the chart. This is done in the updated method of the chart. Change it to a watcher on the data and everything is fine.

changed updated to watch to draw the chart only when the data changes
@ankane
Copy link
Owner

ankane commented May 17, 2018

Thanks @dirk-ecker 👍 I'm able to reproduce the issue, but the PR isn't fixing it. Here's the code I'm using to reproduce.

<div id="app">
  <pie-chart :data="[['Blueberry', 44], ['Strawberry', 23]]" :library="{animation: {duration: 1000}}"></pie-chart>

  {{count}}
</div>

<script>
  var app = new Vue({
    el: "#app",
    data: {
      count: 0
    },
    mounted: function() {
      setInterval(() => {
        this.count += 1
      }, 2000)
    }
  })
</script>

Also, I think we'll need the same for chartOptions.

@dirk-ecker
Copy link
Author

dirk-ecker commented May 18, 2018

Yes sure we should also implement this for the chartOptions and all other props.

It is working if you do not inline the data in the pie chart:

<template>
  <v-container>
    <pie-chart :data="pie" :library="{animation: {duration: 1000}}"></pie-chart>
    {{count}}
  </v-container>
</template>
<script>
export default {
  data () {
    return {
      pie: [['Blueberry', 44], ['Strawberry', 23]],
      count: 0
    }
  },
  mounted: function() {
    setInterval(() => {
      this.count += 1
    }, 2000)
  }
}
</script>

So far I do not know the reason for this behaviour.

@ankane
Copy link
Owner

ankane commented May 26, 2018

Cool, happy to merge once chart options are fixed.

@dirk-ecker
Copy link
Author

okay, chart options are also fixed, have a look ;-)

@ankane
Copy link
Owner

ankane commented May 28, 2018

When I run it on test/index.html, I get TypeError: Cannot read property 'concat' of undefined.

@dirk-ecker
Copy link
Author

Oh the automatic insertion of semicola fails at this point.
I have created a temporary variable to fix this.

@ankane
Copy link
Owner

ankane commented May 30, 2018

Hey @dirk-ecker, when I run this against test/index.html, there are still a lot of updates.

I also created a branch with a different solution that's working for me: https://github.com/ankane/vue-chartkick/compare/less_updates

@ankane
Copy link
Owner

ankane commented May 30, 2018

I've narrowed down the problem. It only happens when you pass a JavaScript object as an inline prop. If you use data for the library option, the problem goes away.

@ankane
Copy link
Owner

ankane commented May 30, 2018

The issue is described here: vuejs/vue#4060

Think a deep equal on data and chartOptions should solve it.

@ankane
Copy link
Owner

ankane commented May 30, 2018

Alright, think the less_updates branch has a solid fix. Please give it a shot when you have some time. Sorry for the noise.

@dirk-ecker
Copy link
Author

Yes it works as expected.
No problem with the noise, makes it clear.
Thank you.

@ankane
Copy link
Owner

ankane commented May 30, 2018

Awesome, merged in d3b0e42, and will push out a new release shortly. Thanks for reporting and helping solve this.

@ankane ankane closed this May 30, 2018
@dirk-ecker
Copy link
Author

dirk-ecker commented Jun 5, 2018 via email

@ankane
Copy link
Owner

ankane commented Jun 5, 2018

This should be solved in 0.3.3, as destroy was added at the chart level. Are you still seeing issues with the latest version?

@dirk-ecker
Copy link
Author

Okay, you are right. I was missing the update. Thank you.

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

Successfully merging this pull request may close these issues.

None yet

2 participants