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.

Sign up for GitHub

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

Add limit props to set height and width of XLine and YLine respectively #152

Merged
merged 3 commits into from
Jul 8, 2019
Merged

Add limit props to set height and width of XLine and YLine respectively #152

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dabcc61
implement xy limit props
tanayv Jun 10, 2019
385003d
update documentation
tanayv Jun 10, 2019
d089a0f
add test cases, better undefined check for XLine
tanayv Jun 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions docs/src/docs/XLine/propDocs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,27 @@
"required": true,
"description": ""
},
"yScale": {
"type": {
"name": "func"
},
"required": false,
"description": "D3 scale for Y axis - provided by XYPlot"
},
"yLimit": {
"type": {
"name": "any"
},
"required": false,
"description": "Limit on the height of the line"
},
"yDomain": {
"type": {
"name": "array"
},
"required": false,
"description": "Y domain of the data as an array - provided by XYPlot"
},
"spacingTop": {
"type": {
"name": "number"
Expand Down
14 changes: 14 additions & 0 deletions docs/src/docs/YLine/propDocs.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,20 @@
"required": true,
"description": ""
},
"xScale": {
"type": {
"name": "func"
},
"required": false,
"description": "D3 scale for X axis - provided by XYPlot"
},
"xLimit": {
"type": {
"name": "any"
},
"required": false,
"description": "Limit on the width of the line"
},
"spacingLeft": {
"type": {
"name": "number"
Expand Down
24 changes: 22 additions & 2 deletions src/XLine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,15 @@ export default class XLine extends React.Component {
*/
xScale: PropTypes.func,
value: PropTypes.any.isRequired,
/**
* D3 scale for Y axis - provided by XYPlot
*/
yScale: PropTypes.func,
yLimit: PropTypes.any,
/**
* The Y domain of the data as an array - provided by XYPlot
*/
yDomain: PropTypes.array,
/**
* Spacing top - provided by XYPlot
*/
Expand Down Expand Up @@ -39,6 +48,9 @@ export default class XLine extends React.Component {
const {
xScale,
value,
yScale,
yLimit,
yDomain,
height,
style,
spacingTop,
Expand All @@ -47,13 +59,21 @@ export default class XLine extends React.Component {
const className = `rct-chart-line-x ${this.props.className}`;
const lineX = xScale(value);

let y1 = -spacingTop;
let y2 = height + spacingBottom;

if (yLimit) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems possible for yLimit to be 0 making this falsey.

y1 = yScale(yDomain[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't this stay -spacingTop?

Copy link
Contributor Author

@tanayv tanayv Jun 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screen Shot 2019-06-12 at 11 23 21 PM

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@tanayv tanayv Jun 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍

y2 = yScale(yLimit);
}

return (
<line
{...{
x1: lineX,
x2: lineX,
y1: -spacingTop,
y2: height + spacingBottom,
y1: y1,
y2: y2,
className,
style
}}
Expand Down
11 changes: 10 additions & 1 deletion src/YLine.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ export default class YLine extends React.Component {
*/
yScale: PropTypes.func,
value: PropTypes.any.isRequired,
/**
* D3 scale for X axis - provided by XYPlot
*/
xScale: PropTypes.func,
xLimit: PropTypes.any,
/**
* Spacing left - provided by XYPlot
*/
Expand Down Expand Up @@ -39,19 +44,23 @@ export default class YLine extends React.Component {
const {
yScale,
value,
xScale,
xLimit,
width,
spacingLeft,
spacingRight,
style
} = this.props;
const className = `rct-chart-line-y ${this.props.className || ""}`;
const lineY = yScale(value);
const lineX =
typeof xLimit === "undefined" ? width + spacingRight : xScale(xLimit);

return (
<line
{...{
x1: -spacingLeft,
x2: width + spacingRight,
x2: lineX,
y1: lineY,
y2: lineY,
className,
Expand Down