Improve themeGet helper

[shawnbot]

I made a suggestion in #368 that I think allows us to at least remove the fallback values from our styled-system themeGet() calls. Rather than this:

import {themeGet} from 'styled-system'
import {colors} from './theme'
const Component = styled('div')`
  color: ${themeGet('colors.green.5', colors.green[5])};
`

We can export a get() function from theme.js using styled-system's own get() function to compute the fallback value:

import {get as getKey, themeGet} from 'styled-system'
export const get = key => themeGet(key, getKey(theme, key))

...which simplifies our components that use themeGet() nicely:

import {get} from './theme'
const Component = styled('div')`
  color: ${get('colors.green.5')};
`

This does allow us to get rid of defaultProps.theme in cases where we only use themeGet(), but other system props will still need theme to be set outside of a ThemeProvider to work properly. 😕

/cc @mxstbr

[emplums]

I think this could work! Regarding leaving theme out of defaultProps, we'll still need that in every component so that the user has access to the theme for color and space props if a ThemeProvider is not provided

[emplums]

The only other issue/thing I'd love to address eventually is that most default values for styled-system props are being set in defaultProps right now, to avoid duplication of styles which happens when we do:

const Link = styled(Base)`
  text-decoration: ${props => (props.underline ? 'underline' : 'none')};
  color: ${themeGet('colors.green.5', colors.green[5])};
  &:hover {
    text-decoration: underline;
    ${hoverColor};
  }
  ${props => (props.is === 'button' ? buttonStyles : '')};
  ${TYPOGRAPHY} ${COMMON};

which makes it impossible to use themeGet (since props are accessible in defaultProps). Curious if this is an okay tradeoff or if @mxstbr has any thoughts!

[mxstbr]

This makes a lot of sense to me. I wish we could remove the theme from defaultProps entirely to be able to support partial theme overrides like <Component theme={{ colors: { green: ['customgreen'] } }} />. @emplums if I understand your comment correctly that would break because the space and color props wouldn't work anymore, right?

Is usage with or without a ThemeProvider the blessed path, i.e. will the ThemeProvider be front-and-center in the setup docs or tucked away in the "custom theming" docs?

[shawnbot]

IMO, it'd be great to be able to add the styled-system prop and its default value in one place. Something like:

import {space} from 'styled-system'
import {withThemeDefault} from './system-props'

const Component = styled.div`
  ${withThemeDefault(space, 2)}
`
Component.propTypes = {
  ...space.propTypes
}

Maybe the helper function takes the fallback value and calls the styled-system function with the right values if the prop isn't set?

import theme from './theme'
export function withThemeDefault(styledFunction, value) {
  // see: https://github.com/jxnblk/styled-system/blob/master/src/index.js#L101
  const prop = Object.keys(styledFunction.propTypes)[0]
  return props => {
    if (prop in props) {
      return props.theme ? styledFunction(props) : styledFunction({theme, ...props})
    } else {
      return styledFunction({theme, [prop]: value})
    }
  }
}

[colebemis]

@shawnbot I like the idea of adding the styled-system prop and its default value in one place 👍 In your example, how do you know which css property you're setting since space could set many properties (e.g. margin-top, margin-right, etc.)?

[emplums]

If we're always setting the theme in defaultProps, we don't need to use themeGet at all - we can just call the theme prop directly since it will always be set.

[emplums]

@shawnbot these solutions are cool, but I'm not sure how necessary they are IMO. Setting up system props, supplying a default, and setting the theme in one function feels a bit too much of a mixture of concerns for me and sort of what I was trying to get away from in the refactor. The defaultProps API is pretty standard and easy for people to jump in and know exactly what's going on.

[emplums]

@mxstbr I was thinking that the ThemeProvider would be an advanced feature that's tucked away a bit. The primary usage of the component library is for GitHub employees to use to build UI in accordance with our design system, so it makes sense that not needing to worry about theming and getting all of our "Primer-y" theme values should be default without any extra configuration. Those who do need to drastically change the style/theme of the components (marketing, site engineering, etc) can still use the components by using our ThemeProvider. For more of a general use component library like Rebass or something like Material, it makes more sense to keep theming and customization front and center.

[emplums]

I added Shawn's original suggestion! closing this issue!