search

pablojim / highcharts-ng

AngularJS directive for Highcharts
MIT License
1.73k stars 463 forks source link

Improve component initialization #621

Closed ngehlert closed 6 years ago

ngehlert commented 6 years ago

What do you think of changing the $onInit into an $onChanges. In the $onChanges you could always create a new Highcharts() object. We have charts with huge amounts of data points, and sometimes Highcharts itself becomes slow in updating the current graph - especially if the entire chart changes, and it is a lot faster to just create a new Highcharts() object. The workaround currently is to wrap everything in a ng-repeat="config in configWrapper" construct so with every configuration you get a new chart.

Also what do you think of adding a parameter to deactivate the changeDetection? Or remove the defaultValue of changeDetection? With huge objects with thousands of data points the comparison can become really slow. The workaround for this is provide an empty function as comparison. But having a native binding just looks nicer.

I would be willing to submit a PR if you agree with the suggestion(s)

pablojim commented 6 years ago

@ngehlert Thanks for the feedback!

There's a balance here to be struck for users with charts with not much data vs users with lots of data. For small amounts of data using chart update can give a nicer experience.

I wonder would a solution be to allow setting global default config for the directive? e.g. setting the change detection strategy and the use of update once for an entire app. Using a provider or similar?

Let me know your thoughts.

ngehlert commented 6 years ago

@pablojim thanks for your quick response hm provider definitely possible, even tho I think it is a little bit overkill. Then I would just add another binding like 'deactivateChangeDetection'. This way you keep the "normal" behavior for users with less data, but advance users can still customize it if they need it.

And the $onInit <--> $onChanges is not affected by this. If you want to update your charts, sure go ahead and update the object reference with new data values. And the change detection will publish it and update the chart. But if you pass in an entirely new Object reference it think re-creating the chart actually makes sense

pablojim commented 6 years ago

Happy to take a pull request for this. With the trade-off discussed, do whatever you think suits best and we can discuss further in the PR.

ngehlert commented 6 years ago

ok awesome. Will do it tomorrow.

edit: https://github.com/pablojim/highcharts-ng/pull/622