feat(Text): New Text component; upgrade to styled-system@v5 #79
Conversation
Codecov Report
@@ Coverage Diff @@
## v5 #79 +/- ##
==========================================
+ Coverage 66.15% 66.37% +0.22%
==========================================
Files 20 21 +1
Lines 458 461 +3
Branches 92 93 +1
==========================================
+ Hits 303 306 +3
Misses 124 124
Partials 31 31
Continue to review full report at Codecov.
|
| @@ -41,12 +42,12 @@ | |||
| "react-popper": "^1.3.3", | |||
| "react-portal": "^4.2.0", | |||
| "react-transition-group": "^2.5.3", | |||
| "styled-system": "^3.2.1" | |||
| "styled-components": "^4.0.0", | |||
There was a problem hiding this comment.
Why did we downgrade styled-components?
| @@ -15,11 +16,18 @@ export default { | |||
| format: 'es', | |||
| }, | |||
| ], | |||
| external: [...Object.keys(pkg.dependencies || {}), ...Object.keys(pkg.peerDependencies || {})], | |||
| external: [ | |||
| ...Object.keys(pkg.dependencies || {}).filter(dep => dep !== 'styled-components'), | |||
There was a problem hiding this comment.
Would it be worthwhile to add a comment about why we're doing this?
| @@ -29,10 +30,11 @@ const StyledAlert = createComponent({ | |||
| }); | |||
|
|
|||
| /** Alerts are typically used to display meaningful copy to users - typically notifying the user of an important message. */ | |||
| const Alert = props => <StyledAlert {...props} />; | |||
| const Alert = React.forwardRef((props, ref) => <StyledAlert {...props} ref={ref} />); | |||
| @@ -22,11 +23,12 @@ const StyledBadge = createComponent({ | |||
| }, | |||
| }); | |||
|
|
|||
| const Badge = props => <StyledBadge {...props} />; | |||
| const Badge = React.forwardRef((props, ref) => <StyledBadge {...props} ref={ref} />); | |||
|
|
||
| Alert.propTypes = { | ||
| variant: PropTypes.string, | ||
| ...propTypes.space, |
| space, | ||
| color, | ||
| typography | ||
| ), |
|
|
||
| test('"color" prop', () => { | ||
| const { asFragment } = renderWithTheme(<Text color="red">Hey!</Text>); | ||
| expect(asFragment()).toMatchSnapshot(); |
There was a problem hiding this comment.
While we're at it, should we add a space prop test as well?
| <> | ||
| <Text>The basic text component renders as a {'<span />'}</Text> | ||
| <Text as="h1">It can render as other tags, like this {'<H1/>'}</Text> | ||
| <Text color="red">It can use colors from the theme</Text> |
There was a problem hiding this comment.
Similar to above, should we add a spacing prop case while we're going to the trouble to illustrate these props, or is it similar enough to the existing spacing props on other components, that there's no need for the additional explanation?
| @@ -27,16 +27,10 @@ export default class ModalDropdownDemo extends React.Component { | |||
| return ( | |||
| <div> | |||
| <Dropdown placement="top" width={250} trigger={<Button variant="success">Open Dropdown</Button>}> | |||
| <Dropdown.Header title="Dropdown" /> | |||
There was a problem hiding this comment.
No more need for the multi section dropdown? Do we want to use the divider here? I think we had added the dropdown to facilitate keyboard accessibility testing.
| ${getColumnWidth(props[breakpoint], theme.gridColumns)}; | ||
| ${props[`${breakpoint}Offset`] !== undefined && getOffset(props[`${breakpoint}Offset`], theme.gridColumns)}; | ||
| } | ||
| `; |
There was a problem hiding this comment.
Could we use breakpoint aliases to generate these media queries?
There was a problem hiding this comment.
I'd rather not touch the implementation in this PR
Summary
New
<Text />component<Text as="h1" fontSize={100} color="primaryLight" />Upgrade styled-system
compose<Box m={{ _: 0, sm: 12 }} />, which says for all screens, apply 0 margin, and for small screens and up, apply a 12px marginClean up theme.js
Clean up
<Column />orderstyle so styled-system can applyorderresponsively, e.g.<Column order={{ _: -1, sm: 1 }} />Fix Dropdown stories