Timeline Component! 🔮

[dmarcey]

Adds a Timeline component

Closes #677

Screenshots

Merge checklist

• Added or updated TypeScript definitions (index.d.ts) if necessary
• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

[vercel]

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-components/ct3tu11dh
✅ Preview: https://primer-components-git-timeline-component.primer.now.sh

[BinaryMuse]

Thanks for tackling this! Added a few notes.

[BinaryMuse]

Just a few more notes, which I'll consolidate here:

• The TypeScript types and the JS types still don't quite agree; since these components are wrappers around existing components, we can use their propTypes directly, similar to how we inherit their props in the TS definitions (and drop the associated system prop definitions).
• Similarly, since these components already inherit from types that implement system props, including e.g. ${FLEX} in their style definition will duplicate some CSS, which you can see in the snapshots. Also, the tests should be updated to ensure all the implemented system props work (so accidental changes in inheritance that break components will also break tests).
• Finally, to avoid the ${props => props.theme.whatever} pattern, wrap the conditional styles in the css helper, which enables use of the standard get(...) pattern.

So as to not ping-pong you back and forth with requested changes, here's a diff that implements these. Feel free to use as much or as little of it as you like.

diff --git a/src/Timeline.js b/src/Timeline.js
index f9dae873..a38f6373 100644
--- a/src/Timeline.js
+++ b/src/Timeline.js
@@ -1,7 +1,7 @@
 import React from 'react'
 import PropTypes from 'prop-types'
-import styled from 'styled-components'
-import {COMMON, FLEX, get} from './constants'
+import styled, {css} from 'styled-components'
+import {get} from './constants'
 import theme from './theme'
 import Box from './Box'
 import Flex from './Flex'
@@ -12,16 +12,15 @@ const Timeline = styled(Flex)`
   flex-direction: column;
   ${props =>
     props.clipSidebar &&
-    `.Timeline-Item:first-child {
-    padding-top: 0;
-  }
-
-  .Timeline-Item:last-child {
-    padding-bottom: 0;
-  }
-`}
-  ${COMMON};
-  ${FLEX};
+    css`
+      .Timeline-Item:first-child {
+        padding-top: 0;
+      }
+
+      .Timeline-Item:last-child {
+        padding-bottom: 0;
+      }
+    `}
 `
 
 Timeline.Item = styled(Flex).attrs(props => ({
@@ -43,24 +42,22 @@ Timeline.Item = styled(Flex).attrs(props => ({
 
   ${props =>
     props.condensed &&
-    `
-    padding-top: ${props.theme.space['1']};
-    padding-bottom: 0;
-    &:last-child {
-      padding-bottom: ${props.theme.space['3']};
-    }
-
-    .TimelineItem-Badge {
-      height: 16px;
-      margin-top: ${props.theme.space['2']};
-      margin-bottom: ${props.theme.space['2']};
-      color: ${props.theme.colors.gray[4]};
-      background-color: ${props.theme.colors.white};
-      border: 0;
-    }
-  `}
-  ${COMMON};
-  ${FLEX};
+    css`
+      padding-top: ${get('space.1')};
+      padding-bottom: 0;
+      &:last-child {
+        padding-bottom: ${get('space.3')};
+      }
+
+      .TimelineItem-Badge {
+        height: 16px;
+        margin-top: ${get('space.2')};
+        margin-bottom: ${get('space.2')};
+        color: ${get('colors.gray.4')};
+        background-color: ${get('colors.white')};
+        border: 0;
+      }
+    `}
 `
 
 const TimelineBadgeInternal = styled(Flex).attrs(props => ({
@@ -78,7 +75,6 @@ const TimelineBadgeInternal = styled(Flex).attrs(props => ({
   border: 2px solid ${get('colors.white')};
   border-radius: 50%;
   flex-shrink: 0;
-  ${COMMON};
 `
 
 Timeline.Badge = props => {
@@ -117,8 +113,7 @@ Timeline.propTypes = {
   children: PropTypes.node,
   clipSidebar: PropTypes.bool,
   theme: PropTypes.object,
-  ...COMMON.propTypes,
-  ...FLEX.propTypes
+  ...Flex.propTypes
 }
 
 Timeline.Item.defaultProps = {
@@ -129,8 +124,7 @@ Timeline.Item.propTypes = {
   children: PropTypes.node,
   condensed: PropTypes.bool,
   theme: PropTypes.object,
-  ...COMMON.propTypes,
-  ...FLEX.propTypes
+  ...Flex.propTypes
 }
 
 Timeline.Badge.defaultProps = {
@@ -140,7 +134,7 @@ Timeline.Badge.defaultProps = {
 Timeline.Badge.propTypes = {
   children: PropTypes.node,
   theme: PropTypes.object,
-  ...COMMON.propTypes
+  ...Flex.propTypes
 }
 
 Timeline.Body.defaultProps = {
@@ -150,7 +144,7 @@ Timeline.Body.defaultProps = {
 Timeline.Body.propTypes = {
   children: PropTypes.node,
   theme: PropTypes.object,
-  ...COMMON.propTypes
+  ...Box.propTypes
 }
 
 Timeline.Break.defaultProps = {
@@ -160,7 +154,7 @@ Timeline.Break.defaultProps = {
 Timeline.Break.propTypes = {
   children: PropTypes.node,
   theme: PropTypes.object,
-  ...COMMON.propTypes
+  ...Box.propTypes
 }
 
 export default Timeline
diff --git a/src/__tests__/Timeline.js b/src/__tests__/Timeline.js
index 5391fd5e..eea146c3 100644
--- a/src/__tests__/Timeline.js
+++ b/src/__tests__/Timeline.js
@@ -1,7 +1,7 @@
 import React from 'react'
 import Timeline from '../Timeline'
 import {render, rendersClass} from '../utils/testing'
-import {COMMON} from '../constants'
+import {COMMON, FLEX, LAYOUT} from '../constants'
 import {render as HTMLRender, cleanup} from '@testing-library/react'
 import {axe, toHaveNoViolations} from 'jest-axe'
 import 'babel-polyfill'
@@ -10,6 +10,8 @@ expect.extend(toHaveNoViolations)
 describe('Timeline', () => {
   it('implements system props', () => {
     expect(Timeline).toImplementSystemProps(COMMON)
+    expect(Timeline).toImplementSystemProps(FLEX)
+    expect(Timeline).toImplementSystemProps(LAYOUT)
   })
 
   it('should have no axe violations', async () => {
@@ -35,6 +37,8 @@ describe('Timeline', () => {
 describe('Timeline.Item', () => {
   it('implements system props', () => {
     expect(Timeline.Item).toImplementSystemProps(COMMON)
+    expect(Timeline.Item).toImplementSystemProps(FLEX)
+    expect(Timeline.Item).toImplementSystemProps(LAYOUT)
   })
 
   it('should have no axe violations', async () => {
@@ -64,6 +68,7 @@ describe('Timeline.Item', () => {
 describe('Timeline.Badge', () => {
   it('implements system props', () => {
     expect(Timeline.Badge).toImplementSystemProps(COMMON)
+    expect(Timeline.Badge).toImplementSystemProps(LAYOUT)
   })
 
   it('should have no axe violations', async () => {
diff --git a/src/__tests__/__snapshots__/Timeline.js.snap b/src/__tests__/__snapshots__/Timeline.js.snap
index 96dfb5ca..606a8e9c 100644
--- a/src/__tests__/__snapshots__/Timeline.js.snap
+++ b/src/__tests__/__snapshots__/Timeline.js.snap
@@ -9,10 +9,6 @@ exports[`Timeline renders 1`] = `
   -webkit-flex-direction: column;
   -ms-flex-direction: column;
   flex-direction: column;
-  display: -webkit-box;
-  display: -webkit-flex;
-  display: -ms-flexbox;
-  display: flex;
 }
 
 <div
@@ -30,10 +26,6 @@ exports[`Timeline renders with clipSidebar prop 1`] = `
   -webkit-flex-direction: column;
   -ms-flex-direction: column;
   flex-direction: column;
-  display: -webkit-box;
-  display: -webkit-flex;
-  display: -ms-flexbox;
-  display: flex;
 }
 
 .c0 .Timeline-Item:first-child {
@@ -78,10 +70,6 @@ exports[`Timeline.Badge renders 1`] = `
   -webkit-flex-shrink: 0;
   -ms-flex-negative: 0;
   flex-shrink: 0;
-  display: -webkit-box;
-  display: -webkit-flex;
-  display: -ms-flexbox;
-  display: flex;
 }
 
 <div
@@ -99,10 +87,6 @@ exports[`Timeline.Item renders 1`] = `
   position: relative;
   padding: 16px 0;
   margin-left: 16px;
-  display: -webkit-box;
-  display: -webkit-flex;
-  display: -ms-flexbox;
-  display: flex;
 }
 
 .c0::before {
@@ -133,10 +117,6 @@ exports[`Timeline.Item renders with condensed prop 1`] = `
   margin-left: 16px;
   padding-top: 4px;
   padding-bottom: 0;
-  display: -webkit-box;
-  display: -webkit-flex;
-  display: -ms-flexbox;
-  display: flex;
 }
 
 .c0::before {

[dmarcey]

Thanks for the diff, @BinaryMuse! That was super helpful - I switched to using the css helper to get rid of the props => props.theme syntax and updated the propTypes.

I don't think that I am able to get rid of the ${COMMON} props from TimelineBadgeInternal and Timeline.Item, however. When I tried to do that, I noticed that the common system props (like bg or pt) stopped working. Does it make sense that I would need to continue to include those?

[emplums]

edit: I refer to custom styles here, by that I mean one-off styles added to a styled-component function that are separate from system props, like everything past the first three lines here:

const TimelineBadgeInternal = styled(Flex).attrs(props => ({
  className: classnames(props.className, 'TimelineItem-Badge')
}))`
  position: relative;
  overflow: hidden;
  z-index: 1;
  width: 32px;
  height: 32px;
  margin-right: ${get('space.2')};
  margin-left: -15px;
  color: ${get('colors.gray.7')};
  border: 2px solid ${get('colors.white')};
  border-radius: 50%;
  flex-shrink: 0;
`

@dmarcey I did some digging and figured out the reason that you need to continue to include them is because the css that you add to TimelineBadgeInteral in styled-components is applied after the CSS created by using one of the ${COMMON} system props when that CSS is created by an extended component. That's also why we always tend to list the system props last in the styled components function, so it always overrides the custom styles included in the component. As you can see, it gets pretty convoluted when extending a component like we're doing here. The order of styles applied in this case would be:

• all custom styles from the Flex component
• system prop styles from the Flex component
• all custom styles from TimelineBadgeInternal component
• If you were to add ${COMMON} to the end of the TimelineBadgeInteral styles, they would be applied at this point (last). you'd see a duplication of styles generated (once for the COMMON implementation in Flex and once for the COMMON implementation in TimelineBadgeInternal

Here's how that looks in the styles produced:

One solution for this particular case is to set the default value for the bg prop for Timeline.Badge to gray.2 instead of applying it in the custom CSS. That will make sure that the background is gray by default, but if someone provides a custom background it gets handled by the Flex component without getting overridden by custom CSS in TimelineBadgeInternal

This has been something lingering in the back of my mind regarding extending components from other components 😬 It sort of breaks down whenever you have "default" styles that override system styles like we do for padding and background color here. We could opt to never extend components within the library to avoid this. Curious if you have any ideas!

[emplums]

Ok messed around a bit with this today and I think this should work as an alternative! :) I moved most of the styles in the styled components function into the Flex component as props. That way they can reliably be overridden if a user provides the same props.

const TimelineBadgeBase = (props) => {
  return (
    <Flex flexShrink={0} className={classnames(props.className, 'TimelineItem-Badge')} color='gray.7' width='32px' height='32px' mr={2} ml='-15px' alignItems='center' justifyContent='center' {...props}>
      {props.children}
    </Flex>
  )
}

Timeline.Badge = styled(TimelineBadgeBase)`
  position: relative;
  overflow: hidden;
  z-index: 1;
  border: 2px solid ${get('colors.white')};
  border-radius: 50%;
`

You could take this even further and completely remove the need for using any custom styles by doing this:

Timeline.Badge = (props) => {
  return (
    <Relative zIndex={1}>
      <Flex flexShrink={0} className={classnames(props.className, 'TimelineItem-Badge')} css={`border-radius: 50%; border: 2px solid ${get('colors.white')}`} overflow='hidden' color='gray.7' width='32px' height='32px' mr={2} ml='-15px' alignItems='center' justifyContent='center' {...props}>
        {props.children}
      </Flex>
    </Relative>
  )
}

The amount of props used in the Flex component feels a little gnarly but not sure if that should stop us from shipping something like that. What do you think?

[emplums]

This could lead to a recommendation for designing components/extending components: "Any styles that correspond to styled-system props should be applied via props instead of custom styles to ensure that styled-system props are applied properly in the consuming application"