Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#15925 closed enhancement (fixed)

[patch][cla] Charting - Grid: alternate background fill & reduce code

Reported by: Mathevet julien Owned by: cjolif
Priority: undecided Milestone: 1.9
Component: Charting Version: 1.8.0
Keywords: Cc: ben hockey
Blocked By: Blocking:

Description

This patch:

  • allow to draw alternate colored background (vFill/hFill options): See [[Img(grid.png)]
  • reduce duplicated code to draw grid lines

Attachments (5)

patch_BackgroundGrid.diff (12.2 KB) - added by Mathevet julien 7 years ago.
grid.png (29.4 KB) - added by Mathevet julien 7 years ago.
patch15925.patch (15.6 KB) - added by cjolif 7 years ago.
base on moogle (CLA) patch, that implements in addition my comments bullet 1, 2, 5, 7 & 8
patch15925.2.patch (16.1 KB) - added by cjolif 7 years ago.
Adding 3 & 4
patch15925.3.patch (18.1 KB) - added by cjolif 6 years ago.
add animation support + fix issue when beginning / end of an axis do not correspond to a tick (empty stripes were rendered)

Download all attachments as: .zip

Change History (17)

Changed 7 years ago by Mathevet julien

Attachment: patch_BackgroundGrid.diff added

Changed 7 years ago by Mathevet julien

Attachment: grid.png added

comment:1 Changed 7 years ago by Mathevet julien

comment:2 Changed 7 years ago by cjolif

Owner: changed from Eugene Lazutkin to cjolif
Status: newassigned

comment:3 Changed 7 years ago by cjolif

Milestone: tbd1.9

comment:4 Changed 7 years ago by cjolif

This looks good. Some small remarks:

  • the patch is missing the documentation of the hFill & vFill properties in the GridCtorArgs object.
  • the patch introduces a module name define(name, []). It should not.
  • some very minor optims from the old code are not here anymore (like not calling getTransformerFromModel several times for the same axis/scaler)
  • SimpleTheme.js "grid" part must be modified to support the new fill style properties.
  • I think we should not draw either lines or rectangles in two totally distinct code paths as you do. We should draw lines if v/hMajor/MinorLines is true as we do today. And rectangles _without_ stroke if v/hFill is here. Otherwise this is confusing as this creates two modes that ignore each others with two ways of setting line colors (regular way + the stroke of the rectangles).
  • Shouldn't all of this work only on major ticks? That would avoid the ticks sorting... And it seems to me that putting fills between each minors tick would bloat the screen? I'm not sure about that one. I wonder if you have the use-case of filling between minor ticks?

Some enhancement suggestions:

  • shouldn't we support filling with more than colors? If we just support color maybe better calling the property color than fill.
  • shouldn't we provide fill & alternateFill instead of forcing no filling on each other interval? That would be more flexible.

comment:5 Changed 7 years ago by ben hockey

Cc: ben hockey added

comment:6 Changed 7 years ago by cjolif

Also I see in the code that v&hStripes properties were already "reserved" for enabling / disabling the feature (that was apparently never implemented). Maybe we could leverage this as well.

comment:7 Changed 7 years ago by cjolif

@moogle, do you have time to update your patch re my comments? If not I'm willing to give it a try.

Changed 7 years ago by cjolif

Attachment: patch15925.patch added

base on moogle (CLA) patch, that implements in addition my comments bullet 1, 2, 5, 7 & 8

Changed 7 years ago by cjolif

Attachment: patch15925.2.patch added

Adding 3 & 4

comment:8 Changed 7 years ago by cjolif

another point missing is the animation of the stripes. Grid lines get animated not stripes. I guess it should be consistent.

comment:9 Changed 7 years ago by Mathevet julien

Sorry I tought it wasn't urgent due to 1.9 release date. I was busy this past weeks. Thank's to make this patch.

comment:10 Changed 7 years ago by cjolif

No pb moogle. This is not urgent per 1.9 deadline, but there are two reasons I'd like to get this in early:

1/ I have some cycles now to code, review... I might not have later on ;)

2/ I have other contributions pending (like bidi mirroring) that need to touch a little bit the code everywhere and so better stabilize it before.

Changed 6 years ago by cjolif

Attachment: patch15925.3.patch added

add animation support + fix issue when beginning / end of an axis do not correspond to a tick (empty stripes were rendered)

comment:11 Changed 6 years ago by cjolif

Resolution: fixed
Status: assignedclosed

In [29865]:

fixes #15925. Implement grid stripes. Based on patch from moogle (CLA).

comment:12 Changed 6 years ago by cjolif

In [29866]:

refs #15925. Use 'render' naming instead of 'draw' which is more inline with charting terminology. Makes method privates.

Note: See TracTickets for help on using tickets.