Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added drawGridLinesOnTopEnabled boolean flag to draw grid lines on top of Bar Charts #2390

Closed

Conversation

JacopoMangiavacchi
Copy link

I had this UI requirements for a projects and added this boolean flag, and relative behaviour, to draw the grid lines above the Bar Charts

@codecov-io
Copy link

Codecov Report

Merging #2390 into master will increase coverage by 0.01%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2390      +/-   ##
==========================================
+ Coverage   19.66%   19.67%   +0.01%     
==========================================
  Files         112      112              
  Lines       13713    13722       +9     
==========================================
+ Hits         2696     2700       +4     
- Misses      11017    11022       +5
Impacted Files Coverage Δ
Source/Charts/Charts/BarLineChartViewBase.swift 23.61% <61.53%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04c5820...24275ae. Read the comment docs.

@liuxuan30
Copy link
Member

liuxuan30 commented Apr 26, 2017

What's this for? Any screenshot to demonstrate? From the code I don't see the difference for grid line in front or background?

@JacopoMangiavacchi
Copy link
Author

Sure thing @liuxuan30 , an image is better than 1000 words ;-)

grid line on top

@liuxuan30
Copy link
Member

😂 however I can't see the grid line difference?

@JacopoMangiavacchi
Copy link
Author

Oh, maybe there's another way but I didn't found an existing settings for having the (white) vertical lines draw on top of the (blue/orange) horizontal lines like in the picture above. I just added this setting and in the draw() method I moved the drawing of these lines after the bar lines if this property is true.

@liuxuan30
Copy link
Member

liuxuan30 commented Apr 27, 2017

Are you saying the white vertical thin lines are grid lines? If so, I think I understand now. The bars are as if cut off into small rects by the grid line.

Well, I feel these grid lines are confusing - the grid lines are interfering reading the chart. Can you give some examples in real world to support this kind of grid lines?

@JacopoMangiavacchi
Copy link
Author

Yes @liuxuan30 . I'm not a designer but as said I just had this UX request to implement a chart like this, with bars cutted off in small rect by the grid line. Other charts frameworks have this feature but I preferred to use Charts so I've implemented this.

@JacopoMangiavacchi
Copy link
Author

Btw, of course this is just a new optional configuration parameter (drawGridLinesOnTopEnabled) where the default value (false) draw a traditional chart with grid lines below the bar line.

@liuxuan30
Copy link
Member

Sure, I see. I will leave this PR to @danielgindi , as I'm not sure if we should add this. I am fine, but adding more 'switches' also needs to be careful

@jjatie
Copy link
Collaborator

jjatie commented Jan 23, 2018

Grid lines should be behind the data. If this is a requirement for your project, that's fine, but it's not something for the shared framework.

@jjatie jjatie closed this Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants