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

spanGaps property for Radar chart #6392

Closed
1 of 3 tasks
stockiNail opened this issue Jul 17, 2019 · 9 comments · Fixed by #6393
Closed
1 of 3 tasks

spanGaps property for Radar chart #6392

stockiNail opened this issue Jul 17, 2019 · 9 comments · Fixed by #6393

Comments

@stockiNail
Copy link
Contributor

Documentation Is:

  • Missing or needed
  • Confusing
  • Not Sure?

New spanGaps dataset property for Radar chart

Checking the documentation, commited into master, I see that new spanGaps dataset property has been added.

The documentation is reporting:

If the value is undefined, spanGaps fallback to the associated chart configuration options. The rest of the values fallback to the associated elements.line.* options.

Unfortunately the spanGaps is not present into elements.line documentation.

Furthermore, adding the property to elements.line, it looks like a duplication for line chart where the same property is defined into configuration itself.

Proposal for Changes

Apart to document spanGaps into elements.line page, I think the spanGaps property at configuration level for line chart should be removed in order to use only 1, into elements.line.

@nagix
Copy link
Contributor

nagix commented Jul 17, 2019

To be exact, the spanGaps option falls back in the following order if undefined:

  • data.datasets[*].spanGaps (per-dataset option)
  • options.datasets.line.spanGaps (per-chart dataset type option)
    • Chart.defaults.radar.datasets.line.spanGaps (per-chart-type dataset type option)
    • Chart.defaults.global.datasets.line.spanGaps (global dataset type option)
  • options.spanGaps (per-chart option)
    • Chart.default.radar.spanGaps (per-chart-type option)
    • Chart.default.global.spanGaps (global option)

The documentation is reporting:

If the value is undefined, spanGaps fallback to the associated chart configuration options. The rest of the values fallback to the associated elements.line.* options.

Unfortunately the spanGaps is not present into elements.line documentation.

As stated in the doc, spanGaps falls back to options.spanGaps, not elements.line.spanGaps in both line and radar charts. However, spanGaps is missing in the configuration options in the radar chart doc, so I created #6393 to fix it.

Edit: Corrected for radar charts

  • data.datasets[*].spanGaps (per-dataset option)
  • options.datasets.radar.spanGaps (per-chart dataset type option)
    • Chart.defaults.radar.datasets.radar.spanGaps (per-chart-type dataset type option)
    • Chart.defaults.global.datasets.radar.spanGaps (global dataset type option)
  • options.spanGaps (per-chart option)
    • Chart.default.radar.spanGaps (per-chart-type option)
    • Chart.default.global.spanGaps (global option)

@stockiNail
Copy link
Contributor Author

@nagix thank you for explanation.

  • options.datasets.line.spanGaps (per-chart dataset type option)
    • Chart.defaults.radar.datasets.line.spanGaps (per-chart-type dataset type option)
    • Chart.defaults.global.datasets.line.spanGaps (global dataset type option)

It's written .datasets. but I think you wanted to write .elements., isn't it?

What it's still not clear, why per-chart option (and for global option) you have got 2 spanGaps properties, one at level 0 and one into elements.line. The fallback could be anyway consistent having only one.

Having a fallback DATASETS - CHART OPTIONS - GLOBAL OPTIONS, my expectation is to have only 1 property for fallback node.

Therefore let me ask you what is the difference (for CHART.JS) setting spanGaps (per-chart option) in options.datasets.line.spanGaps or options.spanGaps?

If I understood correctly, setting options.datasets.line.spanGaps, I'm setting spanGaps for all datasets of chart, and setting options.spanGaps should be same. Maybe I'm wrong.

@benmccann
Copy link
Contributor

@stockiNail see the docs here about dataset configuration to see if it helps clear things up: https://github.com/chartjs/Chart.js/tree/master/docs/configuration#dataset-configuration

@stockiNail
Copy link
Contributor Author

@benmccann thank you, I missed that part of doc. My fault.

Comparing with past, there are 2 new objects to map datasets properteis at options level:

  • per chart: options.datasets[type].*
  • globally: Chart.defaults.global.datasets[type].*

First question: what does [type] mean? Are we talking about elements objects or others?

Forgive me but still have some doubts.

Now let's make an example: property spanGaps.
When I'm configuring a line chart, I have got an options object where I can set the spanGaps property in 3 different places:

  • options.spanGaps
  • options.datasets.line.spanGaps
  • options.elements.line.spanGaps

Second question:; apart of fallback logic, what is the difference to set spanGaps in an object instead of the others, for my chart instance that I'm creating?
If I set options.spanGaps to true, what are the differences on my chart behaviors instead of setting by
options.datasets.line.spanGaps to true or options.elements.line.spanGaps to true ?

@benmccann
Copy link
Contributor

type means chart or dataset type like line, radar, etc.

I'm not particularly familiar with spanGaps or the elements options specifically, but yes I imagine setting any of those to have the same effect if you have just a single chart and dataset.
If you have multiple then different options may give you flexibility to have them applied in just some cases. We may rethink some of the options in v3, but at the moment we haven't had much flexibility to cleanup and remove options if we want to maintain backwards compatibility.

@stockiNail
Copy link
Contributor Author

@benmccann I had the feeling that this new datasets object is going to new version.

type means chart or dataset type like line, radar, etc.

Fine even if it's not clear which properties can be set in this object. The elements ones? Or all others related to a datasets? See the current example, where it describes how to use showLine, it looks like the second one, then all datasets properties.

If you have multiple then different options may give you flexibility to have them applied in just some cases.

As far as I understood, when you create a chart, you have only 1 options instance which configures the whole chart therefore I have the feeling that the property can be set wherever you like with the same effect.

OK, but now it's quite clear. Assuming what I wrote above, I think I won't change anything waiting for the version 3 where some pieces (maintained for backwards compatibility) will be removed.

@nagix
Copy link
Contributor

nagix commented Jul 18, 2019

Because of the historical reason, spanGaps does not fallback to elements.line but options (see #2811). In addition, #5999 added per-dataset type options (options.datasets.radar) which precede per-chart options (options). The complicated part is that both per-dataset and per-chart options fallback to per-chart-type options (Chart.defaults.rader) then global options (Chart.defaults) if undefined.

@stockiNail
Copy link
Contributor Author

@nagix got it!

As far as I understood, curretly spanGaps fallback pass thru chart options anyway. Therefore I'm gonna wait for datasets object implementation, in my project and in the meantime it works.

@nagix @benmccann

Before closing the issue, could you confirm that inside the datasets object i can set all properties of the chart datasets, like backgroundColor, borderColor, borderWidth, scriptable options, and so on?

If my assumption is correct, my hint could be to add into the doc of dataset properties for each chart type that those properties can be set into datasets configuration of the chart and globally.

@benmccann
Copy link
Contributor

Most options can be read from the dataset. Both _resolveDatasetElementOptions and _resolveDataElementOptions use it and those methods are used for reading most options

@etimberg etimberg added this to the Version 2.9 milestone Jul 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants