Add specific class names to x and y axes #191
Conversation
| @@ -31,14 +32,21 @@ const propTypes = { | |||
| ]) | |||
| }; | |||
|
|
|||
| delete propTypes.axisType; | |||
There was a problem hiding this comment.
Using delete prevents optimizations and it's a mutating operation. How about alternatives such as the rest operator or using lodash's omit?
There was a problem hiding this comment.
I'm not sure I understand what you are suggesting with rest parameters? I also haven't heard of delete preventing optimizations? (Though I would be excited to see source saying otherwise)
There was a problem hiding this comment.
Meant to attach references but forgot, here goes. delete causes v8 to deoptimize, the hidden classes optimization.
Here's a related discussion: https://groups.google.com/forum/#!topic/v8-users/zE4cOHBkAnY
and googleapis/google-api-nodejs-client#375 (comment) .
For example with rest: const {axisType, ...propsWithoutAxisType} = Axis.propTypes;
| return ( | ||
| <Axis {...props} /> | ||
| <Axis {...axisProps} /> |
There was a problem hiding this comment.
You can supply additional props here by: <Axis {...props} axisType={DIRECTION.VERTICAL} />.
This might be cleaner as it doesn't require the introduction of a new variable (axisProps vs props).
| const propTypes = { | ||
| ...Axis.propTypes, |
There was a problem hiding this comment.
You can do custom validators, an even better option here might be to return an Error if axisType is specified when using x-axis:
https://facebook.github.io/react/docs/typechecking-with-proptypes.html
| @@ -39,13 +43,17 @@ const propTypes = { | |||
| orientation: React.PropTypes.oneOf([ | |||
| LEFT, RIGHT, TOP, BOTTOM | |||
| ]), | |||
| axisType: React.PropTypes.oneOf([ | |||
| DIRECTION.VERTICAL, DIRECTION.HORIZONTAL | |||
| ]).isRequired, | |||
There was a problem hiding this comment.
Reading this PR in detail, could the axisType a derived from the orientation? For example, would there be a case where a LEFT or RIGHT orientation axis, would have a axisType of HORIZONTAL?
There was a problem hiding this comment.
i guess because they are required that's true
| left, | ||
| top, | ||
| width, | ||
| height, | ||
| orientation, | ||
| title | ||
| } = props; | ||
|
|
||
| const isY = [LEFT, RIGHT].indexOf(orientation) > -1; |
There was a problem hiding this comment.
I typically recommend .includes for bit of added readability, if there are existing usages of .includes, we should use that instead since that would mean existing users have it already set up.
| @@ -19,6 +19,7 @@ | |||
| // THE SOFTWARE. | |||
|
|
|||
| import React from 'react'; | |||
|
|
|||
There was a problem hiding this comment.
whitespace change unnecessary
| @@ -38,7 +39,7 @@ const defaultProps = { | |||
|
|
|||
| function XAxis(props) { | |||
| return ( | |||
| <Axis {...props} /> | |||
| <Axis {...props}/> | |||
| } = props; | ||
|
|
||
| const isY = [LEFT, RIGHT].indexOf(orientation) > -1; |
There was a problem hiding this comment.
I typically recommend .includes for bit of added readability, if there are existing usages of .includes, we should use that instead since that would mean existing users have it already set up.
There was a problem hiding this comment.
I'm totally with you, but there aren't any examples of includes in react-vis, while there are a number of instances of indexOf
There was a problem hiding this comment.
cool, no change here then, 👍 .
| @@ -71,6 +72,10 @@ const defaultProps = { | |||
| orientation: BOTTOM | |||
| }; | |||
|
|
|||
| const predefinedClassName = 'rv-xy-plot__axis'; | |||
| const VERTICAL_CLASS_NAME = 'rv-xy-plot__axis-vertical'; | |||
There was a problem hiding this comment.
--vertical is the correct BEM convention. Do you think we'll get requests for separate classNames for left vs right?
There was a problem hiding this comment.
i doubt it, i've also subverted that potential issue by allowing users to supply their own
| } = props; | ||
|
|
||
| const isY = [LEFT, RIGHT].indexOf(orientation) > -1; |
There was a problem hiding this comment.
isY as a variable name might be a little confusing since there's an attribute and it may not be y
* Add specific class names to x and y axes * remove prop without delete * add examples of using custom classing for axes * respond to comment * respond to moar comment * lint ad clean up * use benv * change variable name
* Add specific class names to x and y axes * remove prop without delete * add examples of using custom classing for axes * respond to comment * respond to moar comment * lint ad clean up * use benv * change variable name
Also allow users to specify custom classnames! Addresses #180