Mixed chart #5134
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,160 @@ | ||
| <!doctype html> | ||
| <html> | ||
|
|
||
| <head> | ||
| <title>Mixed Chart</title> | ||
| <script src="../../dist/Chart.bundle.js"></script> | ||
| <script src="../utils.js"></script> | ||
| <style> | ||
| canvas { | ||
| -moz-user-select: none; | ||
| -webkit-user-select: none; | ||
| -ms-user-select: none; | ||
| } | ||
| div { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| } | ||
| div div { | ||
| flex-grow: 1; | ||
| width: 30%; | ||
| margin: 0.5em; | ||
| height: 15em; | ||
| min-width: 20em; | ||
| } | ||
| </style> | ||
| </head> | ||
|
|
||
| <body> | ||
| <button id="randomizeData">Randomize Data</button> | ||
| <div id="container"> | ||
| </div> | ||
| <script> | ||
| var sets = [ | ||
| { | ||
| parents: ['scatter', 'line', 'bubble', 'bar', 'horizontalBar', undefined], | ||
| childs: [ | ||
| {type:'scatter', color:'#f844'}, | ||
| {type:'scatter', color:'#f844'}, | ||
| {type:'line', color:'#4f84', fill:false}, | ||
| {type:'bubble', color:'#8f44'}, | ||
| {type:'line', color:'#f484'}, | ||
| {type:'bar', color:'#48f4'}, | ||
| {type:'bar', color:'#84f4'} | ||
| ] | ||
| }, { | ||
| parents: ['scatter', 'line', 'bubble', 'bar', 'horizontalBar', undefined], | ||
| childs: [ | ||
| {type:'scatter', color:'#f844'}, | ||
| {type:'scatter', color:'#f844'}, | ||
| {type:'line', color:'#4f84', fill:false}, | ||
| {type:'bubble', color:'#8f44'}, | ||
| {type:'line', color:'#f484'} | ||
| ] | ||
| }, { | ||
| parents: ['scatter', 'line', 'bubble', 'bar', 'horizontalBar', undefined], | ||
| childs: [ | ||
| {type:'scatter', color:'#f844'}, | ||
| {type:'line', color:'#4f84', fill:false}, | ||
| {type:'bubble', color:'#8f44'}, | ||
| {type:'line', color:'#f484'}, | ||
| {type:'bar', color:'#48f4'}, | ||
| {type:'bar', color:'#48f4'}, | ||
| {type:'horizontalBar', color:'#84f4'}, | ||
| ] | ||
| }, { | ||
| parents: ['scatter', 'line', 'bubble', 'bar', 'horizontalBar', undefined], | ||
| childs: [ | ||
| {type:'horizontalBar', color:'#f484'}, | ||
| {type:'scatter', color:'#f844'}, | ||
| {type:'bubble', color:'#8f44'}, | ||
| ] | ||
| } | ||
| ]; | ||
| var size = 8; | ||
| var charts = []; | ||
| sets.forEach(function(set) { | ||
| var parents = set.parents; | ||
| var childs = set.childs; | ||
| parents.forEach(function(parent) { | ||
| var datasets = []; | ||
| var childId = -1; | ||
| var labels = []; | ||
| childs.forEach(function(child) { | ||
| childId++; | ||
| var data = []; | ||
| for (var i = 0 ; i < size ; i++) { | ||
| if (child.type === 'bubble') { | ||
| data.push({x:randomScalingFactor(), y:randomScalingFactor(),r:Math.max(randomScalingFactor()/2, 20)}); | ||
| } else if (child.type === 'scatter') { | ||
| data.push({x:randomScalingFactor(),y:randomScalingFactor()}); | ||
| } else { | ||
| data.push(randomScalingFactor()); | ||
| } | ||
| if (childId === 0) { | ||
| labels.push('label '+(i+1)); | ||
| } | ||
| } | ||
| datasets.push({ | ||
| type: child.type, | ||
| label: child.type, | ||
| borderColor: child.color, | ||
| backgroundColor: child.color, | ||
| data: data, | ||
| }); | ||
| if (child.fill !== undefined) { | ||
| datasets[datasets.length-1].fill = child.fill; | ||
| } | ||
| }); | ||
| var div = document.createElement("div"); | ||
| var canvas = document.createElement("canvas"); | ||
| canvas.id = parent; | ||
| div.appendChild(canvas); | ||
| document.getElementById("container").appendChild(div); | ||
| var childListstr = ''; | ||
| childs.forEach(function(child) { | ||
| childListstr += child.type + ', '; | ||
| }) | ||
| childListstr = childListstr.slice(0,-2); | ||
| charts.push(new Chart(canvas.getContext("2d"), { | ||
| type: parent, | ||
| data: { | ||
| labels: labels, | ||
| datasets: datasets, | ||
| }, | ||
| options: { | ||
| responsive: true, | ||
| title: { | ||
| display: true, | ||
| text: 'Main type : ' + parent + ' | Childs : ' + childListstr, | ||
| }, | ||
| tooltips: { | ||
| mode: 'index', | ||
| intersect: true | ||
| } | ||
| } | ||
| })); | ||
| }); | ||
| }); | ||
| console.log(charts); | ||
|
|
||
| document.getElementById('randomizeData').addEventListener('click', function() { | ||
| charts.forEach(function(chart) { | ||
| chart.data.datasets.forEach(function(dataset) { | ||
| dataset.data = dataset.data.map(function(a) { | ||
| if (a.r) { | ||
| return {x:randomScalingFactor(),y:randomScalingFactor(),r:Math.max(randomScalingFactor()/3, 10)}; | ||
| } else if (a.x) { | ||
| return {x:randomScalingFactor(),y:randomScalingFactor()}; | ||
| } | ||
| return randomScalingFactor(); | ||
| }); | ||
| }); | ||
| chart.update(); | ||
| }); | ||
| console.log(charts); | ||
| }); | ||
| </script> | ||
| </body> | ||
|
|
||
| </html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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' | ||
|
There was a problem hiding this comment. Can you expand the comment to explain why 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) There was a problem hiding this comment. 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: There was a problem hiding this comment. I agree that we should try to have all options supported at the dataset level |
||
| // If the main chart has no type and there's no 'bar' child chart, | ||
| // we set the main chart to 'line' | ||
| // This way, users can use a mixed chart without having to set a main type | ||
| var hasBarType = false; | ||
| if (data && data.datasets) { | ||
| data.datasets.forEach(function(dataset) { | ||
| if (dataset.type === 'bar') { | ||
| hasBarType = true; | ||
| } | ||
| }); | ||
| } | ||
| config.options = helpers.configMerge( | ||
| defaults.global, | ||
| hasBarType ? defaults.bar : defaults[config.type || 'line'], | ||
| config.options || {}); | ||
| if (data && data.datasets) { | ||
| data.datasets.forEach(function(dataset) { | ||
| if (dataset.type !== undefined) { | ||
| config.options = helpers.configMerge(defaults[dataset.type], config.options || {}); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| return config; | ||
| } | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for overriding and returning false here? Shouldn't that have already happened?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what i understand, with mixed chart, the default style applied is the style of the 'main' chart.
And for now, only
barorlineare valid main types.So the option
showLinesis never set tofalse. 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think lines on a scatter chart is something that should be possible and its currently the default behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does that mean a test should be added via another PR to make sure it stays that way in the future ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why scatter charts couldn't display lines, so yes these
lineEnabledchanges should be reverted. We could add a test indeed, but in a different PR.