Opened 3 years ago
Last modified 2 years ago
#19047 new defect
Regression after fixing #18061 "Make theme color cycles consistent across chart plots"
Reported by: | Claude Guyomard | Owned by: | |
---|---|---|---|
Priority: | high | Milestone: | 1.13.1 |
Component: | Charting | Version: | 1.12.2 |
Keywords: | Cc: | ||
Blocked By: | Blocking: |
Description
Dojox Charting
The issue https://bugs.dojotoolkit.org/ticket/18061 was fixed by https://github.com/dojo/dojox/pull/109/. This fix didn't take into account the case of ClusteredColumns?. Files attached show the problem :
ClusteredColumns?-columnInsertion-dojo110.html
Although all the series were iterated from last-to-first, columns are displayed in the right order :
- The 1st series is placed on the left with the the 1st color of the theme,
- The 2nd series is placed after the 1st one with the the 2nd color of the theme,
- and so on...
ClusteredColumns?-columnInsertion-dojo112.html
All the series are now iterated from first-to-last, columns are displayed in the wrong order :
- The 1st series is placed on the RIGHT with the the 1st color of the theme,
- The 2nd series is placed BEFORE the 1st one with the the 2nd color of the theme,
- and so on...
ClusteredColumns?-columnInsertion-dojo112-PATCHED.html
All the series are still iterated from first-to-last, columns are displayed in the right order :
- The 1st series is placed on the left with the the 1st color of the theme,
- The 2nd series is placed after the 1st one with the the 2nd color of the theme,
- and so on...
The reason of this is the incorrect management of the 'z' variable in Columns.js :
The Columns.render(dim, offsets) code is :
... var z = this.series.length; arr.forEach(this.series, function(serie){if(serie.hidden){z--;}}); for(var i = 0; i < this.series.length; i++){ ... if(run.hidden){ run.dyn.fill = theme.series.fill; continue; } z--; ... x: offsets.l + ht(val.x + 0.5) + bar.gap + bar.thickness * z, // line 154, usage of 'z' ... }
It should be :
... var z = 0; // the non-hidden series index // REMOVED : arr.forEach(this.series, function(serie){if(serie.hidden){z--;}}); for(var i = 0; i < this.series.length; i++){ ... if(run.hidden){ run.dyn.fill = theme.series.fill; continue; } // REMOVED z--; ... x: offsets.l + ht(val.x + 0.5) + bar.gap + bar.thickness * z, // OK, z == 0 at first loop ... z++; // ADD HERE. CANNOT BE PLACED IN for(;;) because the 'continue' above }
Attachments (5)
Change History (8)
Changed 3 years ago by
Attachment: | ClusteredColumns-columnInsertion-dojo110.html added |
---|
Changed 3 years ago by
Attachment: | ClusteredColumns-columnInsertion-dojo112.html added |
---|
Changed 3 years ago by
Attachment: | ClusteredColumns-columnInsertion-dojo112-PATCHED.html added |
---|
Changed 3 years ago by
Attachment: | Columns.js added |
---|
comment:1 Changed 3 years ago by
Milestone: | tbd → 1.13.1 |
---|---|
Priority: | undecided → high |
Changed 3 years ago by
Attachment: | changeset-columns.diff added |
---|
comment:2 Changed 3 years ago by
Claude, do you care to open a pull request with this patch per the guidelines at https://github.com/dojo/dojo/blob/master/CONTRIBUTING.md ? This will get the fix reviewed and landed faster, and also gives you credit for your contribution.
Columns.js patch