Fix Caret theme getters #188
Conversation
|
|
||
| function CircleOcticon(props) { | ||
| const {size} = props | ||
| const {icon, ...rest} = props | ||
| return ( | ||
| <FlexContainer {...rest} size={size} alignItems="center" justifyContent="center"> | ||
| <Octicon icon={icon} /> | ||
| <Octicon icon={icon} size={size} /> |
There was a problem hiding this comment.
Are these changes just random cleanup? or are they here from a merge or something?
There was a problem hiding this comment.
This one in particular is random cleanup. I noticed that the Octicon wasn't getting all the props it needs.
|
@emplums my hope was that putting pending tests in place for components that hadn't been migrated, it'd be a single character change to enable the test once they are. 😄 |
| function Caret(props) { | ||
| const {bg} = getBg(props) | ||
| const {borderColor} = getBorderColor(props) | ||
| const {borderWidth} = getBorderWidth(props) |
There was a problem hiding this comment.
Note: when we switch to using the tagged template literals, we can implement all of this differently. For example:
export default styled(RenderSVG)`
pointer-events: none;
position: absolute;
${({edge, align, size}) => getPosition(edge, align, size)}
${({edge, align}) => getMargin(edge, align)}
path:first-child { ${props => {{fill: bg(props).backgroundColor}}) }
path:last-child {
fill: none;
${({borderColor, borderWidth}) => ({stroke: borderColor, strokeWidth: borderWidth})}
}
`
| @@ -32,7 +34,7 @@ expect.extend({ | |||
| const fn = systemProps[name] | |||
| return list.concat(Object.keys(fn.propTypes)) | |||
| }, []) | |||
| const missing = expectedPropKeys.filter(key => !propKeys.has(key)) | |||
| const missing = expectedPropKeys.filter(key => !propKeys.has(key)).filter(key => !ALIAS_PROP_TYPES.includes(key)) | |||
There was a problem hiding this comment.
@emplums this fixes the cryptic missing props in certain calls to expect().toImplementSystemProps(). Some were being set by aliases within styled-system, and they're not needed here.
| @@ -31,4 +25,7 @@ CaretBox.defaultProps = { | |||
| position: 'relative' | |||
| } | |||
|
|
|||
| export default withSystemProps(CaretBox, ['color']) | |||
| // we can set this because it "extends" Box implicitly | |||
There was a problem hiding this comment.
Box isn't a system component yet, but I'll switch out Box for Block in #184
There was a problem hiding this comment.
Actually since this PR is still open, would you mind switching out Box for Block here?
There was a problem hiding this comment.
nevermind! I'm sleepy 😂 Since it's just rendering Box and Box is just rendering Block which does implement system props this is fine
| @@ -31,4 +25,7 @@ CaretBox.defaultProps = { | |||
| position: 'relative' | |||
| } | |||
|
|
|||
| export default withSystemProps(CaretBox, ['color']) | |||
| // we can set this because it "extends" Box implicitly | |||
There was a problem hiding this comment.
Actually since this PR is still open, would you mind switching out Box for Block here?
@emplums I figured out the fix for your weird diff in #184. This is based on #187, but take a look and lmk if you think this looks right. Rather than calling
themeGet(), this uses styled-system'sstyle()function to create style functions that mimic how styled-system creates them. The updated snapshot demonstrates that this works.