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 useTheme hook for emotion-theming #1499

Merged
merged 5 commits into from
Sep 11, 2019
Merged

Add useTheme hook for emotion-theming #1499

merged 5 commits into from
Sep 11, 2019

Conversation

tkh44
Copy link
Member

@tkh44 tkh44 commented Sep 11, 2019

What:
Add useTheme hook to emotion-theming

Why:
Sometimes this is more convenient for theming than relying on the functional argument in the css prop. This is especially true with object styles or projects that have existing styles and are moving to theming. Instead of refactoring every css call to be a function this is a much simpler update.

How:

Checklist:

  • Documentation
  • Tests
  • Code complete
  • Changeset

@changeset-bot
Copy link

changeset-bot bot commented Sep 11, 2019

🦋 Changeset is good to go

Latest commit: c7ac0b6

We got this.

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

@codecov
Copy link

codecov bot commented Sep 11, 2019

Codecov Report

Merging #1499 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
packages/emotion-theming/src/use-theme.js 100% <100%> (ø)

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Besides the one comment, SGTM

@@ -0,0 +1,7 @@
// @flow
import * as React from 'react'
Copy link
Member

Choose a reason for hiding this comment

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

Doing 👇 will mean that emotion-theming won't break for people who aren't on >=16.8.0

Suggested change
import * as React from 'react'
import React from 'react'

Copy link
Member

Choose a reason for hiding this comment

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

@mitchellhamilton import * as React from 'react' was always supported, this ain't nothing to do with react@16.8. And also - the wildcard is somewhat more correct than the default export. Or maybe you were referencing something I'm not aware of.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that it would fail for users who aren't on >=16.8.0 because rollup would change import * as React from 'react' to import { useContext } from 'react' and that export wouldn't exist so it would fail. Though I guess since React is CJS, it wouldn't have failed? I'd still rather leave it as import React from 'react' though just in case someone uses an ESM bundle of React and aliased it or rollup users with rollup-plugin-commonjs because of the namedExports option or something like that.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting edge cases. I guess technically they are possible so yeah - let's keep the default for now.

docs/theming.mdx Outdated Show resolved Hide resolved
@tkh44 tkh44 merged commit bfb4005 into master Sep 11, 2019
@tkh44 tkh44 deleted the use-theme-hook branch September 11, 2019 20:54
@github-actions github-actions bot mentioned this pull request Sep 11, 2019
louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020
* Add useTheme hook for emotion-theming

* Add changeset

* Small fixes to imports and docs

* Update theming.mdx
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

3 participants