Opened 20 months ago

Last modified 8 months 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)

ClusteredColumns-columnInsertion-dojo110.html (3.3 KB) - added by Claude Guyomard 20 months ago.
ClusteredColumns-columnInsertion-dojo112.html (3.3 KB) - added by Claude Guyomard 20 months ago.
ClusteredColumns-columnInsertion-dojo112-PATCHED.html (3.3 KB) - added by Claude Guyomard 20 months ago.
Columns.js (10.0 KB) - added by Claude Guyomard 20 months ago.
changeset-columns.diff (1.1 KB) - added by Claude Guyomard 20 months ago.
Columns.js patch

Download all attachments as: .zip

Change History (8)

Changed 20 months ago by Claude Guyomard

Changed 20 months ago by Claude Guyomard

Changed 20 months ago by Claude Guyomard

Changed 20 months ago by Claude Guyomard

Attachment: Columns.js added

comment:1 Changed 20 months ago by dylan

Milestone: tbd1.13.1
Priority: undecidedhigh

Changed 20 months ago by Claude Guyomard

Attachment: changeset-columns.diff added

Columns.js patch

comment:2 Changed 20 months ago by dylan

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.

Note: See TracTickets for help on using tickets.