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

Use Vue.util.mergeOptions to merge default options with user's options #5

Closed
LinusBorg opened this issue Aug 19, 2016 · 3 comments
Closed

Comments

@LinusBorg
Copy link
Contributor

LinusBorg commented Aug 19, 2016

methods: {
    render (data, options) {

      var mergedOptions = Vue.util.mergeOptions(this.options, options)

      const chart = new Chart(
        this.$els.canvas.getContext('2d'), {
          type: 'bar',
          data: data,
          options: mergedOptions
        }
      )
      chart.generateLegend()
    }
  }

This would allow users to override parts of the defaults without having to copy the rest of the defaults into their component to preserve them.

It would also be possible to introduce a 3rd argument as a flag to decide weither to override or not:

 render (data, options, override = true) {
      var mergedOptions =  override ? options : Vue.util.mergeOptions(this.options, options)
@apertureless apertureless added this to the v1.0.3 milestone Aug 19, 2016
@apertureless
Copy link
Owner

Thanks!
I am gonna look into this.

@apertureless apertureless self-assigned this Aug 19, 2016
@apertureless apertureless removed this from the v1.0.3 milestone Aug 20, 2016
@apertureless
Copy link
Owner

apertureless commented Aug 20, 2016

I've looked into mergeOptions(), but as far as I understand it, it is for merging vue instance options if they conflict with mixins.

When a mixin and the component itself contain overlapping options, they will be “merged” using appropriate strategies. For example, hook functions with the same name are merged into an array so that all of them will be called. In addition, mixin hooks will be called before the component’s own hooks:

So its not merging simple javascript objects.

However it's a good idea to extend the defaultOptions. I will work on that.

But I don't really see a reason for the override property. You always overwrite if you are merging. And you will never get a 'clean' chart, because chartjs sets defaultValues, too.

@LinusBorg
Copy link
Contributor Author

Yeah I should have taken a closer look. for the most part, it would work but it would lead to unexpexetd behaviour when using certain vue-releated key names.

You could e.g. use _.defaultsDeep() or _.merge() from lodash.

apertureless added a commit that referenced this issue Aug 21, 2016
* ➕ Add dependency lodash

* ✨ Add helper function to merge chartOptions

* Change Charts to merge options

* And renamed options data to defaultOptions
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

2 participants