Skip to content
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

Export themeGet #492

Merged
merged 2 commits into from Jul 2, 2019
Merged

Export themeGet #492

merged 2 commits into from Jul 2, 2019

Conversation

colebemis
Copy link
Contributor

@colebemis colebemis commented Jul 2, 2019

Exports the get function as themeGet. Exporting themeGet will make it easier for people to use the Primer theme with styled-components.

Usage

import {themeGet} from '@primer/components'
import styled from 'styled-components'

const Example = styled.div`
  color: ${themeGet('colors.gray.7')};
`

Creating components in this way allows users to override theme values with ThemeProvider if necessary.

Merge checklist

  • Changed base branch to release branch
  • Tested in Chrome
  • Tested in Firefox
  • Tested in Safari
  • Tested in Edge

@nicholaai
Copy link

Why not export as get instead of themeGet? Renaming to themeGet might lead to confusion, as get is a replacement for @styled-system/theme-get's themeGet

@shawnbot
Copy link
Contributor

shawnbot commented Jul 2, 2019

Just spit-balling... would this API be better?

import {theme} from '@primer/components'
export default styled.div`
  color: ${theme.get('colors.gray.7')};
`

Or would you expect theme.get('colors.gray.7') to actually return a value rather than a function? 🤔

@colebemis
Copy link
Contributor Author

@shawnbot Just spit-balling... would this API be better?

That's interesting idea. Although, I do like the idea of the theme being a serializable object. I'd also kind of expect theme.get('colors.gray.7') to return a value.

@nicholaai Why not export as get instead of themeGet? Renaming to themeGet might lead to confusion, as get is a replacement for @styled-system/theme-get's themeGet

I called the export themeGet because you can think of Primer's themeGet function as a replacement for Styled System's themeGet. You can use Primer's themeGet in all the same places you can use Styled System's themeGet. The only is difference is that Primer's themeGet uses Primer's theme values as a built-in fallback values.

I also think the name get could easily be confused with lodash's get function.

@shawnbot
Copy link
Contributor

shawnbot commented Jul 2, 2019

That's interesting idea. Although, I do like the idea of the theme being a serializable object. I'd also kind of expect theme.get('colors.gray.7') to return a value.

Yeah, agreed. I like it the way you've got it, but would love @emplums to weigh in too.

Copy link

@emplums emplums left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I like they API you've got here @colebemis

Can you change the base branch to the 13.2.0 release please?

@colebemis colebemis changed the base branch from master to release-13.2.0 July 2, 2019 21:19
@colebemis colebemis requested a review from emplums July 2, 2019 21:30
@colebemis colebemis merged commit 0027a13 into release-13.2.0 Jul 2, 2019
@colebemis colebemis mentioned this pull request Jul 2, 2019
8 tasks
@emplums emplums deleted the theme-get branch October 22, 2019 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants