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

Fix: area chart stacking doesn't work #1061

Merged
merged 6 commits into from
Jun 1, 2016
Merged

Fix: area chart stacking doesn't work #1061

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c822d1
Force area charts to recalculate on click of legend items.
May 16, 2016
d79d3da
Merge branch 'get-redash-master' into fix/area_plots
May 17, 2016
17427cf
Names the legend item click listener to avoid overriding other places…
May 24, 2016
43b425f
Merge branch 'get-redash-master' of github.com:thoughtworks/redash in…
May 24, 2016
d83c6c4
Raymond/Kumar : Fixes issue where going into edit more would break st…
vorakumar May 26, 2016
3d82b70
Merge branch 'upstream/master' into fix/area_plots
May 31, 2016
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
105 changes: 73 additions & 32 deletions rd_ui/app/scripts/directives/plotly.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,59 +45,82 @@
});
};

var normalAreaStacking = function(seriesList) {
fillXValues(seriesList);
var storeOriginalHeightForEachSeries = function(seriesList) {
_.each(seriesList, function(series) {
if(!_.has(series,'visible')){
series.visible = true;
series.original_y = series.y.slice();
}
});
};

var getEnabledSeries = function(seriesList){
return _.filter(seriesList, function(series) {
Copy link
Member

Choose a reason for hiding this comment

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

You can write this as _.filter(seriesList, 'visible');.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, this would not be the same. Because of Javascript truthiness, when series.visible = 'legendonly' would evaluate to true.

Copy link
Member

Choose a reason for hiding this comment

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

👍

return series.visible === true;
});
};

var initializeTextAndHover = function(seriesList){
_.each(seriesList, function(series) {
series.text = [];
series.hoverinfo = 'text+name';
});
for (var i = 0; i < seriesList.length; i++) {
for (var j = 0; j < seriesList[i].y.length; j++) {
var sum = i > 0 ? seriesList[i-1].y[j] : 0;
seriesList[i].text.push('Value: ' + seriesList[i].y[j] + '<br>Sum: ' + (sum + seriesList[i].y[j]));
seriesList[i].y[j] += sum;
};

var normalAreaStacking = function(seriesList) {
fillXValues(seriesList);
storeOriginalHeightForEachSeries(seriesList);
initializeTextAndHover(seriesList);
seriesList = getEnabledSeries(seriesList);

_.each(seriesList, function(series, seriesIndex, list){
_.each(series.y, function(undefined, yIndex, undefined2){
var cumulativeHeightOfPreviousSeries = seriesIndex > 0 ? list[seriesIndex-1].y[yIndex] : 0;
var cumulativeHeightWithThisSeries = cumulativeHeightOfPreviousSeries + series.original_y[yIndex];
series.y[yIndex] = cumulativeHeightWithThisSeries;
series.text.push('Value: ' + series.original_y[yIndex] + '<br>Sum: ' + cumulativeHeightWithThisSeries);
});
});
};

var lastVisibleY = function(seriesList, lastSeriesIndex, yIndex){
for(; lastSeriesIndex >= 0; lastSeriesIndex--){
if(seriesList[lastSeriesIndex].visible === true){
return seriesList[lastSeriesIndex].y[yIndex];
}
}
};
return 0;
}

var percentAreaStacking = function(seriesList) {
if (seriesList.length === 0) {
return;
}

fillXValues(seriesList);
_.each(seriesList, function(series) {
series.text = [];
series.hoverinfo = 'text+name';
});
for (var i = 0; i < seriesList[0].y.length; i++) {
var sum = 0;
for(var j = 0; j < seriesList.length; j++) {
sum += seriesList[j].y[i];
}
storeOriginalHeightForEachSeries(seriesList);
initializeTextAndHover(seriesList);

for(var j = 0; j < seriesList.length; j++) {
var value = seriesList[j].y[i] / sum * 100;
seriesList[j].text.push('Value: ' + seriesList[j].y[i] + '<br>Relative: ' + value.toFixed(2) + '%');
_.each(seriesList[0].y, function(seriesY, yIndex, undefined){

seriesList[j].y[i] = value;
if (j > 0) {
seriesList[j].y[i] += seriesList[j-1].y[i];
}
}
}
var sumOfCorrespondingDataPoints = _.reduce(seriesList, function(total, series){
return total + series.original_y[yIndex];
}, 0);

_.each(seriesList, function(series, seriesIndex, list){
var percentage = (series.original_y[yIndex] / sumOfCorrespondingDataPoints ) * 100;
var previousVisiblePercentage = lastVisibleY(seriesList, seriesIndex-1, yIndex);
series.y[yIndex] = percentage + previousVisiblePercentage;
series.text.push('Value: ' + series.original_y[yIndex] + '<br>Relative: ' + percentage.toFixed(2) + '%');
});
});
};

var percentBarStacking = function(seriesList) {
if (seriesList.length === 0) {
return;
}

fillXValues(seriesList);
_.each(seriesList, function(series) {
series.text = [];
series.hoverinfo = 'text+name';
});
initializeTextAndHover(seriesList);
for (var i = 0; i < seriesList[0].y.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to remove this and the related code?

Copy link
Member

Choose a reason for hiding this comment

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

@machira @vorakumar ?

Copy link
Author

Choose a reason for hiding this comment

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

@arikfr, No, we didn't mean to do that. It's now fixed.

var sum = 0;
for(var j = 0; j < seriesList.length; j++) {
Expand Down Expand Up @@ -301,6 +324,24 @@
var element = element[0].children[0];
Plotly.newPlot(element, scope.data, scope.layout, scope.plotlyOptions);

element.on('plotly_afterplot', function(d) {
if(scope.options.globalSeriesType === 'area' && (scope.options.series.stacking === 'normal' || scope.options.series.stacking === 'percent')){
$(element).find(".legendtoggle").each(function(i, rectDiv) {
d3.select(rectDiv).on('click', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Using click without a "namespace" mean we override the default?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. We tried using namespace but it didn't work out. The new listener method was never called.

Copy link
Member

Choose a reason for hiding this comment

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

And is there a way to get the previous click handler and call it inside our own?

I'm worried that at some point in the future, Plotly will add some additional logic there that we will miss.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is a risk that if Plotly updates that area, our stuff breaks. On the other hand, such an update may also fix the plotly issue, so we may want to remove this behaviour entirely.
We tried a few things, but it didn't seem to work. Ultimately, we decided not to spend too much time on it since Plotly may be soon replaced by a different charting library. If it becomes an issue, we can revisit and rework this.

Copy link
Member

Choose a reason for hiding this comment

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

OK, sounds reasonable.

var maxIndex = scope.data.length - 1;
var itemClicked = scope.data[maxIndex - i];

itemClicked.visible = (itemClicked.visible === true) ? 'legendonly' : true;
if (scope.options.series.stacking === 'normal') {
normalAreaStacking(scope.data);
} else if (scope.options.series.stacking === 'percent') {
percentAreaStacking(scope.data);
}
Plotly.redraw(element);
});
});
}
});
scope.$watch('layout', function (layout, old) {
if (angular.equals(layout, old)) {
return;
Expand Down