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

Overriding default Highcharts options #6

Closed
sorin-davidoi opened this issue Mar 15, 2016 · 6 comments
Closed

Overriding default Highcharts options #6

sorin-davidoi opened this issue Mar 15, 2016 · 6 comments

Comments

@sorin-davidoi
Copy link
Contributor

Overriding default options seems counterintuitive.

Consider people moving from using Highcharts directly to using this element. They will copy-paste their configuration and expect the chart to look the same, since it is the same configuration. However, they will now have to hunt down the options that have been overwritten, both in the documentation of this element (to find out what has been overwritten) and in Highcharts' documentation, to figure out what was the original default value.

This also make it difficult for people which are already familiar with the Highcharts API, since they are familiar with the default values, and will have to go through the same process.

A really unintuitive example is the legend: it is vertical by default, in contrast to horizontal in Highcharts. Setting legend-position to { align: 'center', verticalAlign: 'bottom', layout: 'horizontal' } is not enough, since we also have to set legend-pos to { x: 0, y: 0 }.

@avdaredevil
Copy link
Owner

Ok, you make valid points as usual @sorin-davidoi. Would you mind making a pull request for this?

If not I'll resolve it by the next release a week or 2 from now.

@sorin-davidoi
Copy link
Contributor Author

Unfortunately, I do not have the time to undertake this right now.

The underlying issue here seems to be whether we want to provide API compatibility with Highcharts. If this is the case, there are some other things to consider.

Firstly, there is no way to set title.align, since our title attribute maps to title.text. This holds true for other attributes (subtitle, for instance). There are also some naming inconsistencies regarding the attributes (chartOptions maps to plotOptions).

Let's consider the current workflow for porting from using Highcharts natively to using this element:

  1. Split your configuration object (the one passed to $('#container').highcharts)
  2. Try to run, fail. Do the following
    1. Look at the documentation and change the attribute names (plotOptions => chartOptions, etc)
    2. Transform stuff like title: { text: 'Awesome chart', align: 'left' } to title: 'Awesome'. Realize that there is no way to specify other attributes than text
  3. Now it runs, but it does not look like it did before:
    1. Hunt down all the parameters and explicitly set the default values Highcharts uses
  4. Remember to call Highcharts.setOptions to get switch back to UTC. This is easy to forget and may come and bite you later.

I think that an ideal workflow would be:

  1. Copy-paste your configuration option

This enables full access to the Highharts API and is straightforward to use. Regarding implementation, we could use deep observers to update the chart. It could look something like this:

properties: {
  config: { type: Object }
},

observers: [
  '_configChanged(config.*)'
],

_configChanged: function(config) {
  // Use config.path to figure out what changed and
  // call the corresponding function in handleChange

  const handleChange = {
    'config.title': function(title) { this.chart.setTitle(title); }
  }
}

What do you think?

@peterchibunna
Copy link

peterchibunna commented Jul 1, 2016

@avdaredevil you can recall when I suggested that you expose the series object to allow us ability to directly add a multi-series data to the chart? This is also because I'm coming from the Highchart full persepective.

I also ran into a use-case where I need to hook into an event of the chart, but your api doesn't seem to allow that. All these make it "no so easy" for people who are familiar with the normal highcharts who want to retain their chart config.

I noticed that you've updated the plugin: I can easily add a multiple series data via the data attribute. Kudos!

@avdaredevil
Copy link
Owner

I completely agree with both of you @sorin-davidoi @peterchibunna. I like the idea of leveraging deep observers for this (in fact it's pobably the best way). What I do have a problem with, is that this is not an ideal way to use an element with Polymer.

I do realize that even after focusing extensively on user experience, advanced users of Highcharts like yourselves will want more from the element than it can do declaratively (as custom programming can go a very long way). In lieu of that, I will draft up a fourth element highcharts-chart-custom, which can only have 1 property: config. This will be implemented customly with updaters, and will be able to have imperative binding against other keys that are not part of the standard highcharts config.

Is this reasonable?

@avdaredevil avdaredevil self-assigned this Oct 5, 2016
@avdaredevil
Copy link
Owner

avdaredevil commented Oct 11, 2016

I will release this element with the public release of Highcharts 5.0.0 as a 2.0.0 Push.
Currently their library is far from production quality.

@avdaredevil
Copy link
Owner

avdaredevil commented Oct 27, 2016

Update: In terms of plotOptions and chartOptions, currently I've added a deprecation error to chartOptions, though it will continue to function up until the new release. [Commit: c848c6f]. Highcharts v5 seems to be much more stable for the migration, and the demo now is the beta view of that upgrade.

@avdaredevil avdaredevil added this to the Release Version 2.0.0 milestone Nov 8, 2016
avdaredevil added a commit that referenced this issue Dec 7, 2016
…to match and work with v5, plotOptions and chartOptions now work with the appropriate, bindings added to zoom, legend options, tooltip options, highchart options, exporting, credits, colorByPoint, removed extraneous properties like legendPos, legendValign, etc. setData updated to also accept a singular object that defines series 1, added getSeries/zoomOut as a function... Ping #6, #14, #13
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

3 participants