Opened 2 years ago

Last modified 11 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 2 years ago.
ClusteredColumns-columnInsertion-dojo112.html (3.3 KB) - added by Claude Guyomard 2 years ago.
ClusteredColumns-columnInsertion-dojo112-PATCHED.html (3.3 KB) - added by Claude Guyomard 2 years ago.
Columns.js (10.0 KB) - added by Claude Guyomard 2 years ago.
changeset-columns.diff (1.1 KB) - added by Claude Guyomard 23 months ago.
Columns.js patch

Download all attachments as: .zip

Change History (8)

Changed 2 years ago by Claude Guyomard

Changed 2 years ago by Claude Guyomard

Changed 2 years ago by Claude Guyomard

Changed 2 years ago by Claude Guyomard

Attachment: Columns.js added

comment:1 Changed 23 months ago by dylan

Milestone: tbd1.13.1
Priority: undecidedhigh

Changed 23 months ago by Claude Guyomard

Attachment: changeset-columns.diff added

Columns.js patch

comment:2 Changed 23 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.