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

Add createTheme helper to emotion-theming #1577

Closed
wants to merge 1 commit into from
Closed

Conversation

Andarist
Copy link
Member

Something we could incorporate into v11 - so it's easier for people to use their own themes which don't conflict with default, libraries built on top of emotion etc

@Andarist Andarist added the WIP label Oct 27, 2019
@Andarist Andarist added this to the v11 milestone Oct 27, 2019
@changeset-bot
Copy link

changeset-bot bot commented Oct 27, 2019

💥 No Changeset

Latest commit: dc29be0

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

@emmatown
Copy link
Member

emmatown commented Nov 5, 2019

TBH, I'm not so sure about this, I don't really see what using this abstraction adds over using context directly. One thing could be the theme merging but that's easy to do outside of the provider by consuming it, merging and passing to the provider and the memoization benefits of this don't really exist because you would have to memoize what you pass to the ThemeProvider anyway. I think the main benefit this would bring would be that it would encourage not using the other theming API because when you say "I have this abstraction" people tend to use it(even if it's not necessarily the right abstraction for them) but I'm still not so sure about it.

@Andarist
Copy link
Member Author

Andarist commented Nov 5, 2019

My intention was to make it easier for people to create custom provider/hook pairs for themes that share the same semantics as the default one. I agree it's easy to do in userland as this is just a thin wrapper around things, but it ensures the logic is "correct" - especially for the provider as hook is just a one-liner.

@emmatown
Copy link
Member

emmatown commented Nov 8, 2019

tbh, I don't think the semantics of the current ThemeProvider are very good, they were made in a pre-hooks(and even pre-createContext) world where it was hard to consume context and memoize things.

Let's compare a usage of the native context API without any abstraction against createTheme

let ThemeContext = React.createContext({
  color: 'green',
  backgroundColor: 'blue'
})
let useTheme = () => React.useContext(ThemeContext)

function SomeComponent({ someColor, children }) {
  let theme = useTheme()
  let newTheme = useMemo(() => ({ ...theme, color: someColor }), [theme, someColor])
  return <ThemeContext.Provider value={newTheme}>{children}</ThemeContext.Provider>
}
let { useTheme, ThemeProvider } = createTheme({
  color: 'green',
  backgroundColor: 'blue'
})

function SomeComponent({ someColor, children }) {
  let newTheme = useMemo(() => ({ color: someColor }), [someColor])
  return <Theme.ThemeProvider theme={newTheme}>{children}</Theme.ThemeProvider>
}

The one with createTheme is certainly shorter but it doesn't really solve any problems. The createTheme example only creates questions for users(how are theme values merged? what if I want to merge themes in a different way to what ThemeProvider does) and requires people to know an extra API.

@Andarist Andarist closed this Nov 24, 2019
@Andarist Andarist deleted the create-theme branch November 24, 2019 12:51
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.

2 participants