-
Notifications
You must be signed in to change notification settings - Fork 538
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
Fix Caret theme getters #188
Changes from 22 commits
39dd6bb
cb59fb6
953adf4
a0ade71
74c74fe
0c4f7da
1e5ffe8
af0a185
707f9e0
cec3939
4509aa3
7ffb519
9790f2e
ed86102
3187f41
a4733e0
be10870
456c9ad
264d338
6e0d39c
23bdb92
1fc3aa9
e2e9471
f70fef8
6791339
721b8e7
e199b21
0ef8868
aa46c4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,28 @@ | ||
import React from 'react' | ||
import PropTypes from 'prop-types' | ||
import Octicon from '@githubprimer/octicons-react' | ||
import FlexContainer from './FlexContainer' | ||
import {withSystemProps} from './system-props' | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these changes just random cleanup? or are they here from a merge or something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This one in particular is random cleanup. I noticed that the Octicon wasn't getting all the props it needs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok cool! |
||
</FlexContainer> | ||
) | ||
} | ||
|
||
CircleOcticon.defaultProps = { | ||
size: 32 | ||
...FlexContainer.defaultProps, | ||
size: 32, | ||
borderRadius: '50%' | ||
} | ||
|
||
CircleOcticon.propTypes = { | ||
icon: Octicon.propTypes.icon | ||
...FlexContainer.propTypes, | ||
icon: Octicon.propTypes.icon, | ||
size: PropTypes.number | ||
} | ||
|
||
export default withSystemProps(CircleOcticon, ['space']) | ||
export default CircleOcticon |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,5 @@ | ||
import {tag, withSystemProps, FLEX_CONTAINER} from './system-props' | ||
import system, {FLEX_CONTAINER, withDefaultTheme} from './system-props' | ||
|
||
const FlexContainer = withSystemProps(tag.div, FLEX_CONTAINER) | ||
const FlexContainer = system({is: 'div', display: 'flex'}, ...FLEX_CONTAINER) | ||
|
||
FlexContainer.defaultProps = { | ||
display: 'flex' | ||
} | ||
|
||
export default FlexContainer | ||
export default withDefaultTheme(FlexContainer) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import {tag, withSystemProps, FLEX_ITEM} from './system-props' | ||
import {withSystemProps, FLEX_ITEM} from './system-props' | ||
|
||
const FlexItem = withSystemProps(tag.div, FLEX_ITEM) | ||
const FlexItem = withSystemProps('div', FLEX_ITEM) | ||
|
||
export default FlexItem |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,21 @@ | ||
import React from 'react' | ||
import {theme, CaretBox} from '..' | ||
import {CaretBox} from '..' | ||
import {render} from '../utils/testing' | ||
|
||
describe('CaretBox', () => { | ||
it('is a system component', () => { | ||
expect(CaretBox.systemComponent).toEqual(true) | ||
}) | ||
|
||
it('renders a <Caret> in <Box> with relative positioning', () => { | ||
expect(render(<CaretBox />)).toMatchSnapshot() | ||
}) | ||
|
||
it('passes the "borderColor" prop to both <Box> and <Caret>', () => { | ||
expect(render(<CaretBox borderColor="red.5" theme={theme} />)).toMatchSnapshot() | ||
expect(render(<CaretBox borderColor="red.5" />)).toMatchSnapshot() | ||
}) | ||
|
||
it('passes the "bg" prop to both <Box> and <Caret>', () => { | ||
expect(render(<CaretBox bg="red.5" theme={theme} />)).toMatchSnapshot() | ||
expect(render(<CaretBox bg="red.5" />)).toMatchSnapshot() | ||
}) | ||
}) |
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.
Note: when we switch to using the tagged template literals, we can implement all of this differently. For example: