Fix: area chart stacking doesn't work #1061
Conversation
This fixes issue #948 where stacked area plots do not update once series are enabled/disabled using the legend. It also fixes the behaviour of percentage stack area plots, so as to have similar behaviour to percentage stacked bar charts.
| }; | ||
|
|
||
| var getEnabledSeries = function(seriesList){ | ||
| return _.filter(seriesList, function(series) { |
There was a problem hiding this comment.
You can write this as _.filter(seriesList, 'visible');.
There was a problem hiding this comment.
Actually, this would not be the same. Because of Javascript truthiness, when series.visible = 'legendonly' would evaluate to true.
|
Overall looks good. Did you test it with a chart having multiple types of series (bars & line, for example)? |
| element.on('plotly_afterplot', function(d) { | ||
| if(scope.options.globalSeriesType === 'area' && (scope.options.series.stacking === 'normal' || scope.options.series.stacking === 'percent')){ | ||
| d3.selectAll('.legendtoggle').each(function(d, i) { | ||
| // override plotly's default legend item on-click behaviour |
There was a problem hiding this comment.
What's the default behavior? Do we "un-override" it when the types changes?
There was a problem hiding this comment.
Thank you for the suggestion. We went back and took a look at it. It turns out we can use a custom listener through d3, and remove it when chart type changes.
… the event may be used. Also removes the event listener when type of chart changes.
|
It looks like multiple series (bar and Area) breaks the stacking. I will keep looking at it, and update you if I find a workaround. |
|
@machira ok, let me know if you find a solution. We can merge as is if you prefer, because it's still better than current situation. |
…acking. - selects legend divs from current graph only (instead of selecting from the entire dom)
|
Hey Arik, we have updated the PR with some new changes. Some additional tests here showed some edge cases we hadn't thought of- such as edit mode. The most recent push works as expected. And multiple series (area and line) also seem to work appropriately. |
| @@ -121,7 +144,6 @@ | |||
| angular.module('plotly', []) | |||
| .constant('ColorPalette', ColorPalette) | |||
| .directive('plotlyChart', function () { | |||
| var bottomMargin = 50; | |||
There was a problem hiding this comment.
Did you mean to remove this and the related code?
There was a problem hiding this comment.
@arikfr, No, we didn't mean to do that. It's now fixed.
|
@vorakumar thanks! I appreciate the effort you put here. |
|
major work has been done by @machira |
|
By "you" I meant all of you at ThoughtWorks :-) |
|
I have seen splunk do this in a different way that might be worth considering:
When you hover over the legend it just highlights the slice that you are analysing:
In saying that, I'm happy with what you guys have suggested. |
# Conflicts: # rd_ui/app/scripts/directives/plotly.js
|
Merged 🎉 |
Fix: area chart stacking doesn't work
This PR addresses issue #948 as recommended by Plotly maintainers here.
Specifically, we recalculate the data appearance when the legend is clicked. This ensures that we show existing series at the updated heights.
We also fix the behaviour of percentage area plots. Here, if the legend is clicked to disable a given series, the visualization is refreshed so that only enabled series are plotted. The percentage total is still calculated over the all series. This is designed to be consistent with the percentage bar charts, where after disabling a given series, the plotted visualizations add up to less than 100%.