BorderBox updates #329
BorderBox updates #329
Conversation
|
This pull request is automatically deployed with Now. |
src/BorderBox.js
Outdated
| @@ -8,7 +9,7 @@ const BorderBox = withSystemProps( | |||
| borderColor: 'gray.2', | |||
| borderRadius: 1 | |||
| }, | |||
| LAYOUT | |||
| [...LAYOUT, boxShadow] | |||
There was a problem hiding this comment.
since borderColor, borderRadius, and border are already set up with defualts in lines 5-10 we don't need to re-apply them here, only boxShadow needed to be added
There was a problem hiding this comment.
LAYOUT is just a list of strings so it might make sense to add the boxShadow system prop using a string instead of a function here. Making boxShadow a string would also fix the issue with boxShadow being included in HTML markup, which I believe is happening because BorderBox.propTypes doesn't include the boxShadow prop so it isn't being added to the blacklist.
| [...LAYOUT, boxShadow] | |
| [...LAYOUT, 'boxShadow'] |
src/CircleOcticon.js
Outdated
| <Flex {...rest} size={size} alignItems="center" justifyContent="center"> | ||
| <Octicon icon={icon} size={size} /> | ||
| </Flex> | ||
| <BorderBox bg={bg} overflow='hidden' border='none' size={size} borderRadius='50%'> |
There was a problem hiding this comment.
This feels kind of lame but 🤷♀️
| @@ -117,6 +117,7 @@ exports[`Dropdown matches the snapshots 1`] = ` | |||
| </svg> | |||
| </summary> | |||
| <div | |||
| boxShadow="small" | |||
There was a problem hiding this comment.
can't figure out why this is being left behind on the HTML markup 🤔 any thoughts @shawnbot ?
There was a problem hiding this comment.
I think this change will remove boxShadow=... from the markup
| @@ -8,7 +8,7 @@ const BorderBox = withSystemProps( | |||
| borderColor: 'gray.2', | |||
| borderRadius: 1 | |||
| }, | |||
| LAYOUT | |||
| [...LAYOUT, 'boxShadow'] | |||
There was a problem hiding this comment.
One thing to note here is that system-components infers styled-system props from the default props object ({is: 'div', ...}), which is why we don't have to explicitly list borders, borderColor, and borderRadius here. We might want to do that anyway, since (IIRC) that behavior wasn't carried over to @rebass/components, which we plan to (eventually) upgrade to in #240.
There was a problem hiding this comment.
This looks great, thanks for this! I think it makes a lot of sense not to overload Box with all those props. I left one note about explicitly listing the BorderBox system-props, but that's linked from #240 now so we could address it when we get around to that work if you'd rather get this out as-is.
This PR removes
borders, borderColor, borderRadius, boxShadowprops from theLAYOUTgroup of props, and places them in the BorderBox component instead.Merge checklist