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

Update highcharts-ng.js #15

Closed
wants to merge 1 commit into from
Closed

Update highcharts-ng.js #15

wants to merge 1 commit into from

Conversation

borgateo
Copy link

@borgateo borgateo commented Sep 3, 2013

Added credits option

Added credits option
@pablojim
Copy link
Owner

pablojim commented Sep 3, 2013

Hi just wondering why this needed. See: http://jsfiddle.net/pablojim/Hjdnw/4/

@borgateo
Copy link
Author

borgateo commented Sep 4, 2013

Hi Barry,

I think because it doesn't follow the highcharts API - http://api.highcharts.com/highcharts#credits.

Why "credits" needs to stay within "options" and, for instance, "title" and "loading" not?
Maybe I am wrong but it looks inconsistent.

Cheers,
Matteo

On Tuesday, 3 September 2013 at 17:35, Barry Fitzgerald wrote:

Hi just wondering why this needed. See: http://jsfiddle.net/pablojim/Hjdnw/4/


Reply to this email directly or view it on GitHub (#15 (comment)).

@pablojim
Copy link
Owner

pablojim commented Sep 4, 2013

Maybe I can give it a better explanation

$scope.chartConfig = {
   options: {...}, //highcharts options - using standard highcharts config
   //other "dynamic" options
   title: {...}
   series [...]
}

So consider the above snippet. In chartConfig the options property is a standard highcharts options object. e.g. anything you can pass into ``new Highcharts.Chart(options);` works here.

This options object is watched for changes. When something changes here the whole chart is refreshed.

The other dynamic properties are ones that we can change without affecting the whole chart - using the api at http://api.highcharts.com/highcharts#Chart e.g. if you change the title we can call chart.setTitle and not have to reload the whole chart. Splitting them out from the main options object means we can watch them separately.

So in your specific case if highcharts introduced a chart.setCredit method then I think it would be appropriate to split it out. Until then there is no advantage to having it as a separate property.

Hope this makes sense!

@borgateo
Copy link
Author

borgateo commented Sep 5, 2013

Makes sense. I am gonna close the pull request.

@borgateo borgateo closed this Sep 5, 2013
@pablojim
Copy link
Owner

pablojim commented Sep 5, 2013

Thanks Matteo

pablojim referenced this pull request Dec 7, 2013
…ple directives on one page

- Multiple directives on one page did modify the defaultOptions array
- This commit moves the default options into the getMergedOptions function to avoid overriding of default properties
@pykler
Copy link

pykler commented Dec 7, 2013

Thanks ... it is now clear why you did it that way. I am wondering if there is an "equally clean" way of keeping those two things separate but still using highcharts original config.

Thinking out loud and maybe get things started ... storing all the keys that require a re-render will be less clean, is there a way to get the best of both?

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

3 participants