-
Notifications
You must be signed in to change notification settings - Fork 534
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.
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
Refactor Block border props #104
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left a few comments, otherwise 👍
src/Block.js
Outdated
@@ -3,6 +3,20 @@ import PropTypes from 'prop-types' | |||
import classnames from 'classnames' | |||
import {mapWhitespaceProps, oneOrMoreOf, stylizer} from './props' | |||
|
|||
const borderColors = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we import these from the theme file instead? (And add them to the theme file if they aren't already there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good call! Done.
src/Block.js
Outdated
'red-light', | ||
'yellow' | ||
] | ||
|
||
const styleProps = ['width', 'minWidth', 'maxWidth', 'height', 'minHeight', 'maxHeight'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we want a constants file to start storing things like these? Can be done in a different PR
|
||
const {style} = stylize(rest) | ||
|
||
return ( | ||
<Tag | ||
className={classnames( | ||
className, | ||
getBorderClass(border), | ||
getBorderClass(border, borderColor), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does passing borderColor here just insure that if someone passes a borderColor
but no border
prop, the border still gets applied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. The idea was that you would only have to pass, say, borderColor='green'
to get a green border, rather than border borderColor='green'
, which feels redundant to me.
@@ -48,11 +77,13 @@ const Block = props => { | |||
|
|||
Block.propTypes = { | |||
bg: PropTypes.string, | |||
border: oneOrMoreOf(PropTypes.oneOfType([PropTypes.string, PropTypes.bool, PropTypes.number])), | |||
border: PropTypes.oneOfType([PropTypes.bool, oneOrMoreOf(PropTypes.oneOf(['top', 'right', 'bottom', 'left']))]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be border: PropTypes.oneOfType([PropTypes.bool, oneOrMoreOf(['top', 'right', 'bottom', 'left']))
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. This is saying, "either a boolean, one of these values, or an array of these values."
Updates to the Block (and Box) border-related props:
border
now only accepts a boolean or one or more edge names (top
,right
,bottom
, orleft
).borderColor
accepts one of the colors in ourborder-*
utility class list.round
is nowborderRadius
.Here's what that looks like:
Notes
borderRadius
values are multipliers as in margin and padding, not pixel values.Fixes #74.