Closed Minishlink closed 6 years ago
Do you have an example chart that exposes the problem that you are trying to address in this pull request?
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 |
---|---|
![]() |
![]() |
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)?
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:
Sure, I'll also do this for Bar and Scatterplot 👍
Before | After |
---|---|
![]() |
![]() |
![]() |
![]() |
Thank you. I appreciate you contributing this improvement to the library.
Thank you for contributing a pull request.
Please ensure that you have signed the CLA.