Add limit props to set height and width of XLine and YLine respectively #152
Conversation
There was a problem hiding this comment.
Hey @tanayv this looks great, just a few comments.
Also, let's add some tests to YLine.spec.js and XLine.spec.js to make sure it works as expected now and in the future :)
src/XLine.js
Outdated
| let y2 = height + spacingBottom; | ||
|
|
||
| if (yLimit) { | ||
| y1 = yScale(yDomain[0]); |
There was a problem hiding this comment.
Why can't this stay -spacingTop?
There was a problem hiding this comment.
So with y1 set as -spacingTop, the line begins from the top instead of the bottom. I assumed since the value is within the domain, the line should begin from the lower bound of the domain. Let me know if that's not what you had in mind/if there's a better implementation for this.
There was a problem hiding this comment.
Gotcha! I think we're missing the spacingBottom as well so we need to account for that if users pass that in. Is it possible, for consistency, for the YLine to always grow from the top or bottom (the latter makes more sense to me). So if there's a yLimit the only thing that would be altered is y2 where y1 should stay yScale(yDomain[0]) - spacingBottom?
There was a problem hiding this comment.
Hey @krissalvador27, sorry for the delay in making these changes. Finally had the chance to get to them.
For the XLine to always consistently grow from the bottom (ie. by fixing the value of y1 as yScale(yDomain[0]) - spacingBottom), I will also have to account for the XLines created by the XGrid, which is a child of the XAxis component.
As of now, the XAxis component doesn't accept the yDomain as a prop from the XYPlot, and so the XLines that make the grid do not have the prop yDomain available. So should I go ahead and add yDomain and yScale as props for both XAxis and XGrid to make all lines (including the grid lines) grow from the bottom?
There was a problem hiding this comment.
Hey @tanayv!
Thanks for catching that! XYPlot should hand yDomain and yScale down to all its children, so XAxis should easily accept it. I do worry that the complexity and scope is growing though. I think we can go with your original logic to accomplish the original XLine and YLine height and width limits and consistently have XLines grow from the top/limit point. 👍
src/XLine.js
Outdated
| let y1 = -spacingTop; | ||
| let y2 = height + spacingBottom; | ||
|
|
||
| if (yLimit) { |
There was a problem hiding this comment.
Seems possible for yLimit to be 0 making this falsey.
|
Thanks for the comments. I'll add the tests and address the comments with an updated PR soon! |
|
Hey @krissalvador27, I've added the test cases for the new props, and fixed the undefined check on XLine. For the logic, I stuck to having only the lines with xLimit in their props to grow from the bottom, while all other XLines consistently grow from the top. Also, I set Thanks! |
…ly (#152) * implement xy limit props * update documentation * add test cases, better undefined check for XLine
Adds the height/width limit props discussed in #149