#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)
Change History (17)
Changed 8 years ago by
Attachment: | patch_BackgroundGrid.diff added |
---|
Changed 8 years ago by
comment:2 Changed 8 years ago by
Owner: | changed from Eugene Lazutkin to cjolif |
---|---|
Status: | new → assigned |
comment:3 Changed 8 years ago by
Milestone: | tbd → 1.9 |
---|
comment:4 Changed 8 years ago by
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 8 years ago by
Cc: | ben hockey added |
---|
comment:6 Changed 8 years ago by
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 8 years ago by
@moogle, do you have time to update your patch re my comments? If not I'm willing to give it a try.
Changed 8 years ago by
Attachment: | patch15925.patch added |
---|
base on moogle (CLA) patch, that implements in addition my comments bullet 1, 2, 5, 7 & 8
comment:8 Changed 8 years ago by
another point missing is the animation of the stripes. Grid lines get animated not stripes. I guess it should be consistent.
comment:9 Changed 8 years ago by
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 8 years ago by
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 8 years ago by
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)