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.

Sign up for GitHub

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

Jump to bottom

Mixed chart #5134

Closed
wants to merge 18 commits into from
Closed

Mixed chart #5134

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
619d1f6
Better mixed graph support
loicbourgois Jan 10, 2018
ee8e195
Set back lineEnabled()
loicbourgois Jan 11, 2018
76b13d1
Move bar scale default value check to getIndexScale()
loicbourgois Jan 11, 2018
cd75f9b
Improved mixed charts with support for empty main type
loicbourgois Jan 12, 2018
1a32e89
Add example for mixed charts
loicbourgois Jan 12, 2018
7cdd680
Add example for mixed charts
loicbourgois Jan 12, 2018
a01f580
Rollback controller.bar.js
loicbourgois Jan 12, 2018
042920e
Update mixed chart example
loicbourgois Jan 12, 2018
05669ed
Add more samples + Remove duplicated code
loicbourgois Jan 13, 2018
2f9727b
Always merge child options to parent
loicbourgois Jan 13, 2018
6e84627
Merge branch 'master' of https://github.com/chartjs/Chart.js into mixed
loicbourgois Jan 14, 2018
de80075
Add test + Generalize controller and sample
loicbourgois Jan 14, 2018
f24d979
Add test + Generalize controller and sample
loicbourgois Jan 14, 2018
8ea9ee6
Make sure the all the chart options are taken into account
loicbourgois Jan 14, 2018
32f14aa
Update example
loicbourgois Jan 14, 2018
892dc15
Simplify merging
loicbourgois Jan 14, 2018
25119d4
Revert changes for scatter and line controllers
loicbourgois Jan 14, 2018
780f4eb
Add check for type
loicbourgois Jan 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions samples/charts/combo-bar-line.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,21 @@
</head>

<body>
<div style="width: 75%">
<div style="width: 60%">
<canvas id="canvas"></canvas>
</div>
<button id="randomizeData">Randomize Data</button>
<div style="width: 60%">
<canvas id="canvas2"></canvas>
</div>
<script>
var chartData = {
labels: ["January", "February", "March", "April", "May", "June", "July"],
datasets: [{
type: 'line',
type: 'scatter',
label: 'Dataset 1',
borderColor: window.chartColors.blue,
borderWidth: 2,
fill: false,
data: [
randomScalingFactor(),
randomScalingFactor(),
Expand All @@ -38,9 +40,11 @@
randomScalingFactor()
]
}, {
type: 'bar',
type: 'line',
label: 'Dataset 2',
backgroundColor: window.chartColors.red,
borderColor: window.chartColors.red,
fill: false,
borderWidth: 2,
data: [
randomScalingFactor(),
randomScalingFactor(),
Expand All @@ -50,8 +54,6 @@
randomScalingFactor(),
randomScalingFactor()
],
borderColor: 'white',
borderWidth: 2
}, {
type: 'bar',
label: 'Dataset 3',
Expand Down Expand Up @@ -85,6 +87,22 @@
}
}
});
var ctx2 = document.getElementById("canvas2").getContext("2d");
window.myMixedChart2 = new Chart(ctx2, {
type: 'line',
data: chartData,
options: {
responsive: true,
title: {
display: true,
text: 'Chart.js Combo Bar Line Chart'
},
tooltips: {
mode: 'index',
intersect: true
}
}
});
};

document.getElementById('randomizeData').addEventListener('click', function() {
Expand All @@ -94,6 +112,7 @@
});
});
window.myMixedChart.update();
window.myMixedChart2.update();
});
</script>
</body>
Expand Down
2 changes: 1 addition & 1 deletion samples/samples.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
title: 'Radar',
path: 'charts/radar.html'
}, {
title: 'Combo bar/line',
title: 'Combo bar/line/scatter',
path: 'charts/combo-bar-line.html'
}]
}, {
Expand Down
15 changes: 11 additions & 4 deletions src/controllers/controller.bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ var defaults = require('../core/core.defaults');
var elements = require('../elements/index');
var helpers = require('../helpers/index');

defaults._set('bar', {
var defaultBar = {
hover: {
mode: 'label'
},
Expand All @@ -30,7 +30,8 @@ defaults._set('bar', {
type: 'linear'
}]
}
});
};
defaults._set('bar', defaultBar);

defaults._set('horizontalBar', {
hover: {
Expand Down Expand Up @@ -284,7 +285,14 @@ module.exports = function(Chart) {
* @private
*/
getIndexScale: function() {
return this.getScaleForId(this.getIndexScaleId());
var scale = this.getScaleForId(this.getIndexScaleId());
if (scale.options.categoryPercentage === undefined) {
scale.options.categoryPercentage = defaultBar.scales.xAxes[0].categoryPercentage;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
if (scale.options.barPercentage === undefined) {
scale.options.barPercentage = defaultBar.scales.xAxes[0].barPercentage;
}
return scale;
},

/**
Expand Down Expand Up @@ -427,7 +435,6 @@ module.exports = function(Chart) {
var range = options.barThickness === 'flex'
? computeFlexCategoryTraits(index, ruler, options)
: computeFitCategoryTraits(index, ruler, options);

var stackIndex = me.getStackIndex(datasetIndex, me.getMeta().stack);
var center = range.start + (range.chunk * stackIndex) + (range.chunk / 2);
var size = Math.min(
Expand Down
11 changes: 8 additions & 3 deletions src/controllers/controller.line.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ module.exports = function(Chart) {
var scale = me.getScaleForId(meta.yAxisID);
var i, ilen, custom;
var dataset = me.getDataset();
var showLine = lineEnabled(dataset, options);
var showLine = this.lineEnabled(dataset, options);

// Update Line
if (showLine) {
Expand Down Expand Up @@ -287,7 +287,7 @@ module.exports = function(Chart) {

helpers.canvas.clipArea(chart.ctx, area);

if (lineEnabled(me.getDataset(), chart.options)) {
if (this.lineEnabled(me.getDataset(), chart.options)) {
meta.dataset.draw();
}

Expand Down Expand Up @@ -328,6 +328,11 @@ module.exports = function(Chart) {
model.backgroundColor = me.getPointBackgroundColor(point, index);
model.borderColor = me.getPointBorderColor(point, index);
model.borderWidth = me.getPointBorderWidth(point, index);
}
},

lineEnabled: function(dataset, options) {
return lineEnabled(dataset, options);
},

});
};
6 changes: 5 additions & 1 deletion src/controllers/controller.scatter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Copy link
Member

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?

Copy link
Contributor Author

@loicbourgois loicbourgois Jan 12, 2018 •
edited
Loading

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 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.

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Contributor Author

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 ?

Copy link
Member

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?

Copy link
Member

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 lineEnabled changes should be reverted. We could add a test indeed, but in a different PR.

lineEnabled: function() {
return false;
},
});

};