Skip to content
This repository has been archived by the owner on Sep 17, 2021. It is now read-only.

Fix grid axis appearing on top of the lines #197

Merged
merged 3 commits into from
Oct 1, 2017

Conversation

Minishlink
Copy link
Contributor

Thank you for contributing a pull request.

Please ensure that you have signed the CLA.

  • I have signed the CLA

@marzolfb
Copy link
Contributor

Do you have an example chart that exposes the problem that you are trying to address in this pull request?

@Minishlink
Copy link
Contributor Author

Sure, simply add gridColor: 'red' to one of the line graphs in either axisX or axisY, and change the color to a lighter one like white so that you can spot if the grid axis overlaps the lines and regions.

Before After
image image

@marzolfb
Copy link
Contributor

marzolfb commented Oct 1, 2017

The answer to this may be obvious but I will ask anyways - did you consider moving this:

<Axis key="x" scale={chart.xscale} options={options.axisX} chartArea={chartArea} />
<Axis key="y" scale={chart.yscale} options={options.axisY} chartArea={chartArea} />

before this:

{regions}
{areas}
{lines}
{points}

in Line.js vs. the added complexity of splitting out "GridAxis" from Axis and moving it before {regions}as you did?
I'm sure you probably considered this - I would like to understand what the downside is.

(Side note for comparison: Interestingly enough in Bar.js, Axis comes before {lines} while Scatterplot.js is the opposite - the ordering is certainly inconsistent among chart types)

Based on the side note above, if we address the first point above, would you be willing to fix this in the remainder of the chart types that use Axis (Bar & Scatterplot)?

@Minishlink
Copy link
Contributor Author

Thanks for the careful review!

My reasoning was that in Axis you also have ticks and labels, and you probably don't want these to be in the background:
image

Sure, I'll also do this for Bar and Scatterplot 👍

@Minishlink
Copy link
Contributor Author

Before After
image image
image image

@marzolfb
Copy link
Contributor

marzolfb commented Oct 1, 2017

Thank you. I appreciate you contributing this improvement to the library.

@marzolfb marzolfb merged commit 3267c55 into capitalone:master Oct 1, 2017
@Minishlink Minishlink deleted the fix/grid branch October 1, 2017 20:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants