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

Remove default theme prop #1094

Merged
merged 20 commits into from
Mar 3, 2021
Merged

Remove default theme prop #1094

merged 20 commits into from
Mar 3, 2021

Conversation

colebemis
Copy link
Contributor

@colebemis colebemis commented Feb 27, 2021

Problem

We currently set the default theme prop of every component to our Primer theme. We did this to allow consumers to use components in their app without needing to wrap everything in a ThemeProvider. However, setting the default theme prop of components can sometimes override the theme provided by the ThemeProvider and cause unexpected theming issues which will prevent us from implementing color modes.

Solution

This PR removes the default theme prop from every component.

Consumers will now need to wrap their app in a ThemeProvider component and use the Primer theme:

import {ThemeProvider} from 'styled-components'
import {theme} from '@primer/components'
const App = props => {
  return (
    <div>
      <ThemeProvider theme={theme}>
        <div>your app here</div>
      </ThemeProvider>
    </div>
  )
}

TODO

  • Remove default theme prop from every component
  • Update tests
  • Update docs
  • Add changeset
  • Write PR description

@changeset-bot
Copy link

changeset-bot bot commented Feb 27, 2021

🦋 Changeset detected

Latest commit: da668d4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/components Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Feb 27, 2021

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

🔍 Inspect: https://vercel.com/primer/primer-components/4NgEKUST5Qrq7GejYkuoTswBiY3d
✅ Preview: https://primer-components-git-cb-remove-default-theme-prop-primer.vercel.app

@vercel vercel bot temporarily deployed to Preview February 27, 2021 02:02 Inactive
@colebemis colebemis marked this pull request as draft February 27, 2021 02:04
@vercel vercel bot temporarily deployed to Preview March 1, 2021 21:17 Inactive
@vercel vercel bot temporarily deployed to Preview March 1, 2021 21:53 Inactive
@vercel vercel bot temporarily deployed to Preview March 1, 2021 23:04 Inactive
@vercel vercel bot temporarily deployed to Preview March 1, 2021 23:09 Inactive
@vercel vercel bot temporarily deployed to Preview March 1, 2021 23:24 Inactive
@vercel vercel bot temporarily deployed to Preview March 1, 2021 23:34 Inactive
@vercel vercel bot temporarily deployed to Preview March 1, 2021 23:42 Inactive
@vercel vercel bot temporarily deployed to Preview March 1, 2021 23:46 Inactive
@colebemis colebemis changed the title [WIP] Remove default theme prop Remove default theme prop Mar 1, 2021
@colebemis colebemis marked this pull request as ready for review March 1, 2021 23:47
@colebemis colebemis requested a review from emplums March 1, 2021 23:48
Copy link

@bscofield bscofield left a comment

Choose a reason for hiding this comment

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

Looks good to my naive eyes!

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.

Left a couple comments, but nothing blocking. One other general question I have is whether or not we should set up BaseStyles to render a ThemeProvider and take a theme prop? We might be able to pass the Primer theme into the ThemeProvider if the user doesn't pass in a theme prop, and handle the color modes switching there too? Not sure if that's an approach that's already been considered, might be missing something.

const {borderColor} = getBorderColor(props)
const {borderWidth} = getBorderWidth(props)
const theme = React.useContext(ThemeContext)
const propsWithTheme = {...props, theme: props.theme ?? theme}
Copy link

Choose a reason for hiding this comment

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

Can you explain why this is necessary? If the theme doesn't exist in props why would ThemeContext still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since Caret is not a styled component, we need to explicitly get the theme from context and use it as a fallback when evaluating the system props in the lines below (i.e. getBg()).

@@ -157,7 +156,7 @@ const PaginationContainer = styled.nav`
`

export type PaginationProps = {
theme: object
theme?: object
Copy link

Choose a reason for hiding this comment

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

Is it still necessary to directly pass theme down to Box below on line 192? Shouldn't Box get theme as a prop if it's wrapped by a theme provider?

Copy link
Contributor Author

@colebemis colebemis Mar 3, 2021

Choose a reason for hiding this comment

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

Since we still support the theme prop in all components, we still want the theme prop to override the theme from the ThemeProvider. In this PR, we're just making sure that we're not accidentally overriding the ThemeProvider out of the box.

I'm open to removing the theme prop entirely and make people use a ThemeProvider to change the theme but that's a much bigger change that's out of scope for this PR.

Co-authored-by: emplums <emplums@github.com>
@vercel vercel bot temporarily deployed to Preview March 3, 2021 04:00 Inactive
@colebemis
Copy link
Contributor Author

colebemis commented Mar 3, 2021

@emplums

One other general question I have is whether or not we should set up BaseStyles to render a ThemeProvider and take a theme prop? We might be able to pass the Primer theme into the ThemeProvider if the user doesn't pass in a theme prop, and handle the color modes switching there too? Not sure if that's an approach that's already been considered, might be missing something.

I'd rather not make that change in this PR since we'll be revisiting the ThemeProvider/BaseStyles API as part of milestone 2 next week. But I agree that our ThemeProvider API should use the Primer theme by default and provide an easy way to select a color mode.

I'm also hesitant to combine BaseStyles (as it's currently implemented) and ThemeProvider because BaseStyles renders a div which might make it difficult to use the combined ThemeProvider/BaseStyles component where other providers are usually rendered in applications. For example, I remember @jclem mentioning that BaseStyles was tricky to deal with. I'll do more investigation as part of milestone 2 of theming epic 😄

@jclem
Copy link
Contributor

jclem commented Mar 3, 2021

BaseStyles was tricky to deal with because it doesn't expose the ability to have any control over the styling of div that it outputs and we had been using it in the midst of a fairly complex display: flex layout. I don't think combining ThemeProvider and BaseStyles would necessarily make it more difficult, though.

As @emplums pointed out at the time, I think we are probably using BaseStyles in a place in our application structure it isn't intended to be, anyway, so in this case don't put too much weight on our experience with it.

@emplums
Copy link

emplums commented Mar 3, 2021

That's fair @colebemis just wanted to make sure we were still thinking about it!

@colebemis
Copy link
Contributor Author

Ah, thanks for the context, @jclem! That's helpful to know.

@vercel vercel bot temporarily deployed to Preview March 3, 2021 14:55 Inactive
@vercel vercel bot temporarily deployed to Preview March 3, 2021 15:01 Inactive
@colebemis colebemis merged commit d155389 into main Mar 3, 2021
@colebemis colebemis deleted the cb/remove-default-theme-prop branch March 3, 2021 15:55
@github-actions github-actions bot mentioned this pull request Mar 3, 2021
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.

4 participants