Mixed chart #5134

loicbourgois · 2018-01-11T01:32:04Z

As mentioned in #4184, #4587, #4096, the mixed chart functionality is a bit buggy : it only allows line charts to be added to a bar chart.

Original comment

I found a way to display line, bar, scatter on a chart of type line or bar.
I'm afraid it doesn't address the root problem, but it's a net improvement.
You can find a working example here : https://codepen.io/anon/pen/zpWMOj

More mixed chart

I made changes to initConfig in the core controller, to merge options from the child charts in the parent chart.
Supported parents are : line, bar, undefined
Supported childs are : scatter, line, bubble, bar
Here is an example with the changes until commit 042920e : mixed
You can compare it to the current behavior : 2.7.1

package-lock.json

Bar works as parent and child Line works as parent and child Scatter works as child only

etimberg · 2018-01-11T22:41:21Z

src/controllers/controller.scatter.js

@@ -37,6 +37,10 @@ defaults._set('scatter', {
 module.exports = function(Chart) {

 	// Scatter charts use line controllers
-	Chart.controllers.scatter = Chart.controllers.line;
+	Chart.controllers.scatter = Chart.controllers.line.extend({


What's the reason for overriding and returning false here? Shouldn't that have already happened?

From what i understand, with mixed chart, the default style applied is the style of the 'main' chart.
And for now, only bar or line are valid main types.
So the option showLines is never set to false. You'd have to do it manually in the child chart.
You can try commenting the line n°8187 and n°19040 in this codepen
And you can check the console for Chart.config.data.datasets[0].showLine.

But now that I think about it, is being able to show lines in a scatter chart a wanted feature ?
If so, then this overriding would break it.

I think lines on a scatter chart is something that should be possible and its currently the default behaviour

So does that mean a test should be added via another PR to make sure it stays that way in the future ?

I would think adding a test to ensure that a scatter chart can optionally use a line or not is a good idea. @simonbrunel thoughts?

I don't see why scatter charts couldn't display lines, so yes these lineEnabled changes should be reverted. We could add a test indeed, but in a different PR.

etimberg · 2018-01-11T22:42:05Z

src/controllers/controller.bar.js

-			return this.getScaleForId(this.getIndexScaleId());
+			var scale = this.getScaleForId(this.getIndexScaleId());
+			if (scale.options.categoryPercentage === undefined) {
+				scale.options.categoryPercentage = defaultBar.scales.xAxes[0].categoryPercentage;


[nit] I would put defaultBar.scales.xAxes[0] into a local variable to save a few bytes

samples/charts/mixed-chart.html

src/core/core.controller.js

benmccann · 2018-01-14T16:50:20Z

src/core/core.controller.js

@@ -31,10 +31,31 @@ module.exports = function(Chart) {
 		data.datasets = data.datasets || [];
 		data.labels = data.labels || [];

+
+		// Merge options for mixed charts
+		// If there is at least one 'bar' chart, main type is set to 'bar'


Can you expand the comment to explain why If there is at least one 'bar' chart, main type is set to 'bar'

One thing I'm wondering about is if there's a more generic way to do this. I'm worried that this won't work if you want to mix a financial chart and line chart for example (https://github.com/chartjs/chartjs-chart-financial)

I share the same concerns: wouldn't be better to add support for bar specific options (scale) at the dataset level and avoid touching the options logic? Actually, I think that can cause breaking changes, especially the logic that merges all dataset defaults on top of each others. I'm not even sure it works great since the last dataset (options) overwrites the previous ones.

For example, with the following types: line > scatter, the value of showLines will be false but scatter > line generates showLines: true (not consistent). Same for line > bubble, the tooltip options will break for the line.

I agree that we should try to have all options supported at the dataset level

simonbrunel · 2018-01-14T18:31:38Z

I don't know exactly what are the "blocking" issues that make mixed charts impossible, but unfortunately, charts are not consistent in their default options and I don't think there is a magic way to generate the "good" defaults for any mixed charts (e.g. can't merge legendCallback, tooltips.callbacks.label, etc.).

IMO, if no global type is given, then only the global defaults should be merged and the dataset.type should become mandatory. The user would have to provide the correct options that works great with his datasets and on our side we should make sure that we can handle mixed dataset types.

benmccann · 2018-01-14T21:04:53Z

The are two main issues with mixed charts.

The first is that we have scatter and line charts which are the same exact thing, but with different default values of showLines. It's problematic that we try to set this variable both via chart type and via a config option.

The second is that categoryPercentage and barPercentage do not have default values unless the overall chart type is bar. That means that you cannot have a line chart with bar dataset because those values will be undefined. It currently works only if you swap them (i.e. bar chart with line dataset)

simonbrunel · 2018-01-14T21:53:56Z

Are these really blocking issues?

categoryPercentage and barPercentage can be explicitly defined or set as scale defaults
showLines is supported per dataset as showLine (fiddle)

With 2.7.1, mixed line + bar + scatter + bubble seems to work (fiddle). Though, it only works if the chart type is line or bar, that would need to be investigated. But it doesn't seem better with the suggested changes?

benmccann · 2018-01-14T23:40:07Z

I've explicitly defined categoryPercentage and barPercentage in the past when I want to use a mixed chart. However, it took me half an hour of debugging to realized I needed to do this. Hopefully we can make it easier for users. I would suggest that we move the default values for these config options from controller.bar.js to core.scale.js

benmccann · 2018-01-14T23:45:13Z

It looks like the fix of providing default categoryPercentage and barPercentage fixes some mixed charts with bars in the sample provided here, but not all. There's an additional issue, which is that the bar chart calls scale.getPixelForValue(null, i, datasetIndex) and each scale has a different signature for getPixelForValue

Bar charts work with time scales and category scales today:

src/scales/scale.time.js:		getPixelForValue: function(value, index, datasetIndex) {
src/scales/scale.logarithmic.js:		getPixelForValue: function(value) {
src/scales/scale.category.js:		getPixelForValue: function(value, index) {
src/scales/scale.linear.js:		getPixelForValue: function(value) {

I think we'd need to finish the implementation of linear and logarithmic charts getPixelForValue functions to make bar charts work with those scales.

loicbourgois · 2018-01-15T00:40:12Z

I updated the merging of options, removed the changes to scatter and line controllers, and added a new test.
The test however doesn't seem to fail. I'm using expect(meta.data[0].draw.calls.count()).toBe(1);
What would be the appropriate way to verify that all the charts are actually displaying ?
With these latest commits, here are fiddles to compare it to master :
Master + parent defined
Mixed + parent defined
Mixed + parent undefined
Combinations that work in mixed and not master

bar > scatter ;
bubble > horizontalBar ;
bubble > bar ;
bubble > line ;
bubble > scatter ;
line > bar ;
scatter > bar ;
scatter (but the line is showing) > line ;

From a user point of view, I don't think I should be required to set a main type if all my childs have a type. And I shouldn't have to specify some options that should have a default value already.

benmccann · 2018-01-15T01:41:24Z

I tend to agree with @simonbrunel that this approach becomes really difficult to understand the code to know which order options get applied, which override which, etc. I think your first approach was closer

I think we should try to move some chart options to be dataset or scale options instead. E.g. see my last two comments for an approach with bar charts. And here's a PR to fix the line / scatter issue: #5151

benmccann · 2018-01-27T06:37:04Z

I talked to Simon on Slack today about this. He suggested the right long term solution is the one outlined in #5191 and that he will hopefully try to see if it's feasible if he has time

loicbourgois · 2018-01-27T21:06:27Z

It sounds like a better solution indeed.
Will this allow to have an undefined main type ?

simonbrunel · 2019-05-21T09:55:33Z

Thanks @loicbourgois! I'm closing this PR because I think #5999 will help handling mixed chart in a better way than the typesByPriotity approach suggested here which doesn't seem a scalable, stable and durable solution.

nazariyv · 2019-07-25T15:18:18Z

Can the scatter plot take the x-values that are different to the line chart, but are on the same scale as the line chart? i.e. assume you have x-axes range for the line chart to be [0.5, 1.0, 2.0, 2.5, 3.0] and for the scatter chart, one of the points takes an x value of 0.65. As far as I can tell, it gets moved to 0.5...

benmccann · 2019-07-25T15:23:38Z

@nazariyv let's keep the discussion of that question on #6409. thanks!

benmccann reviewed Jan 11, 2018

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

loicbourgois added 2 commits January 11, 2018 10:00

Better mixed graph support

619d1f6

Bar works as parent and child Line works as parent and child Scatter works as child only

Set back lineEnabled()

ee8e195

loicbourgois force-pushed the mixed branch from 3017467 to ee8e195 Compare January 11, 2018 10:03

Move bar scale default value check to getIndexScale()

76b13d1

etimberg reviewed Jan 11, 2018

View reviewed changes

loicbourgois added 4 commits January 12, 2018 02:53

Improved mixed charts with support for empty main type

cd75f9b

Add example for mixed charts

1a32e89

Add example for mixed charts

7cdd680

Rollback controller.bar.js

a01f580

loicbourgois changed the title ~~Mixed chart (line+bar+scatter) with line or bar as main type~~ Mixed chart Jan 12, 2018

Update mixed chart example

042920e

benmccann reviewed Jan 13, 2018

View reviewed changes

samples/charts/mixed-chart.html Outdated Show resolved Hide resolved

benmccann reviewed Jan 13, 2018

View reviewed changes

src/core/core.controller.js Outdated Show resolved Hide resolved

benmccann reviewed Jan 13, 2018

View reviewed changes

src/core/core.controller.js Outdated Show resolved Hide resolved

benmccann reviewed Jan 13, 2018

View reviewed changes

src/core/core.controller.js Outdated Show resolved Hide resolved

loicbourgois added 5 commits January 13, 2018 08:17

Add more samples + Remove duplicated code

05669ed

Always merge child options to parent

2f9727b

Merge branch 'master' of https://github.com/chartjs/Chart.js into mixed

6e84627

Add test + Generalize controller and sample

de80075

Add test + Generalize controller and sample

f24d979

benmccann reviewed Jan 14, 2018

View reviewed changes

Make sure the all the chart options are taken into account

8ea9ee6

Update example

32f14aa

Simplify merging

892dc15

loicbourgois mentioned this pull request Jan 14, 2018

Add tests related to showLines for controller.scatter #5150

Merged

Revert changes for scatter and line controllers

25119d4

Add check for type

780f4eb

benmccann mentioned this pull request Jan 22, 2018

Better handling of mixed line / scatter charts #5151

Closed

etimberg mentioned this pull request Feb 10, 2018

Mixed charts issue #5260

Closed

benmccann added the status: stale label Aug 12, 2018

benmccann mentioned this pull request May 7, 2019

Bar options should not be defined on scale #6249

Merged

simonbrunel closed this May 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mixed chart #5134

Mixed chart #5134

loicbourgois commented Jan 11, 2018 •

edited

Loading

etimberg Jan 11, 2018

loicbourgois Jan 12, 2018 •

edited

Loading

loicbourgois Jan 12, 2018

etimberg Jan 12, 2018

loicbourgois Jan 13, 2018

etimberg Jan 13, 2018

simonbrunel Jan 14, 2018

etimberg Jan 11, 2018

benmccann Jan 14, 2018

simonbrunel Jan 14, 2018

benmccann Jan 14, 2018

simonbrunel commented Jan 14, 2018

benmccann commented Jan 14, 2018

simonbrunel commented Jan 14, 2018

benmccann commented Jan 14, 2018

benmccann commented Jan 14, 2018 •

edited

Loading

loicbourgois commented Jan 15, 2018 •

edited

Loading

benmccann commented Jan 15, 2018 •

edited

Loading

benmccann commented Jan 27, 2018

loicbourgois commented Jan 27, 2018

simonbrunel commented May 21, 2019

nazariyv commented Jul 25, 2019 •

edited

Loading

benmccann commented Jul 25, 2019

Mixed chart #5134

Mixed chart #5134

Conversation

loicbourgois commented Jan 11, 2018 • edited Loading

Original comment

More mixed chart

Choose a reason for hiding this comment

loicbourgois Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonbrunel commented Jan 14, 2018

benmccann commented Jan 14, 2018

simonbrunel commented Jan 14, 2018

benmccann commented Jan 14, 2018

benmccann commented Jan 14, 2018 • edited Loading

loicbourgois commented Jan 15, 2018 • edited Loading

benmccann commented Jan 15, 2018 • edited Loading

benmccann commented Jan 27, 2018

loicbourgois commented Jan 27, 2018

simonbrunel commented May 21, 2019

nazariyv commented Jul 25, 2019 • edited Loading

benmccann commented Jul 25, 2019

loicbourgois commented Jan 11, 2018 •

edited

Loading

loicbourgois Jan 12, 2018 •

edited

Loading

benmccann commented Jan 14, 2018 •

edited

Loading

loicbourgois commented Jan 15, 2018 •

edited

Loading

benmccann commented Jan 15, 2018 •

edited

Loading

nazariyv commented Jul 25, 2019 •

edited

Loading