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

Added support for passing theme to functions in array css prop #1130

Merged
merged 6 commits into from Dec 2, 2019

Conversation

jtmthf
Copy link
Contributor

@jtmthf jtmthf commented Dec 22, 2018

What: When passing an array to the css-prop, functions accepting the theme from emotion-theming are now acceptable as array items.

Why: This addresses #997. I find that there are many use cases where this is helpful. One example would be when composing a number of style utility functions that may or may not read from the theme. For my use case in particular, I'm working on a reimplementation of Tailwind CSS using emotion. This is what one example of usage looks like right now.

<div
  css={theme => [
    display('flex')
    bg('white')(theme),
    mx("auto")(theme),
    maxW('sm')(theme),
    shadow('lg')(theme),
    rounded('lg')(theme),
    { overflow: "hidden" }
  ]}
/>

With this change, the example can be rewritten as:

<div
  css={[
    display('flex')
    bg('white'),
    mx("auto"),
    maxW('sm'),
    shadow('lg')
    rounded('lg'),
    { overflow: "hidden" }
  ]}
/>

In addition to being shorter, the advantage here is that the developer doesn't need to know if a value in the css-prop is a function that needs to be passed the theme.

How: The change was implemented by now wrapping all uses of the array css prop in ThemeContext.Consumer. It could potentially be more optimized to check if the array contains a function first before wrapping in the consumer. In the render method, if the css prop is an array, it is now mapped and for each item that is an array it is passed the theme.

Checklist:

  • Documentation
  • Tests
  • Code complete
  • TypeScript typings

@codecov
Copy link

codecov bot commented Dec 22, 2018

Codecov Report

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

Impacted Files Coverage Δ
packages/core/src/jsx.js 100% <100%> (ø) ⬆️

@codecov
Copy link

codecov bot commented Dec 22, 2018

Codecov Report

Merging #1130 into next will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
packages/babel-plugin-emotion/src/index.js 94.04% <100%> (+0.22%) ⬆️
packages/babel-plugin-emotion/src/core-macro.js 100% <100%> (ø) ⬆️
packages/core/src/jsx.js 100% <100%> (ø) ⬆️
packages/core/src/global.js 97.56% <100%> (ø) ⬆️

@luishdz1010
Copy link

This would be truly helpful, right now object styles break encapsulation since you need to know whether they are functions or objects

@simonflk
Copy link

Thanks for this 👍 What can we do to get this merged?

I was about to submit a very similar change when I noticed your PR. Only difference with mine is that it doesn't pull in the theme if none of the styles are functions. e.g:

<div css={[{ color: 'white' }, css`background-color: hotpink;`]}>foo</div>

@jtmthf
Copy link
Contributor Author

jtmthf commented Mar 28, 2019

@simonflk I actually have the feature you described implemented locally, but not yet committed to this branch. I still need to add documentation and TypeScript typings, but am holding off until I get confirmation that this is something the project owner's want.

@chibicode
Copy link

chibicode commented Apr 11, 2019

👍 This is exactly what I need! Would love to see this merged.

Until this is merged, here's what I'm doing to implement something like this:
#1130 (comment)

Essentially I'm creating another context just underneath ThemeProvider which generates helper functions using theme and makes them accessible to child components via a context.

// Top level - generateThemeHelpers(theme) creates an object containing
// helper functions like display, bg, etc which depend on theme
<ThemeProvider theme={theme}>
  <ThemeHelperContext.Provider value={generateThemeHelpers(theme)}>
    {/* ... */}
  </ThemeHelperContext.Provider>
</ThemeProvider>

const generateThemeHelpers = theme => ({
  display: (/* ... */) => { return css`/* ... */` }
  // etc...
})

// Each component
// Instead of importing these helper functions, get them from context
const { display, bg, mx, maxW, shadow, rounded } = useContext(ThemeHelperContext)

<div
  css={[
    display('flex')
    bg('white'),
    mx("auto"),
    maxW('sm'),
    shadow('lg')
    rounded('lg'),
    { overflow: "hidden" }
  ]}
/>

@a-type
Copy link

a-type commented May 2, 2019

I'd love to see this as well. It's critical to a project I'm working on where I want to be able to compose in user-provided styles and allow the ability to utilize the theme.

@jckw
Copy link

jckw commented Jul 31, 2019

Would be amazing to get this merged! Any timeline?

@pettersamuelsen
Copy link

What's the status on this?

@Andarist
Copy link
Member

We'll consider supporting this in v11 - I'm currently fixing the stuff that can be fixed as part of v10 and once I'm done I will start working on v11 (which won't have major breaking changes). There is not that much left to do, I have a somewhat good pace - but doing it all in my free time so it still might take a few weeks.

# Conflicts:
#	packages/core/src/jsx.js
@changeset-bot
Copy link

changeset-bot bot commented Nov 24, 2019

🦋 Changeset is good to go

Latest commit: 94c8599

We got this.

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

@Andarist Andarist changed the base branch from master to next November 24, 2019 19:48
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 24, 2019

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 94c8599:

Sandbox Source
Emotion Configuration

@Andarist
Copy link
Member

I got this working, made additional changes to babel plugin and Global's styles prop (so its behavior match css prop). I think it's done and would consider it a good change overall - still need @mitchellhamilton approval though.

@emmatown emmatown merged commit c672175 into emotion-js:next Dec 2, 2019
@github-actions github-actions bot mentioned this pull request Dec 2, 2019
louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020
…on-js#1130)

* Added support passing theme to functions in array css prop

* Add changeset

* Adjust how arrays passed to css prop are transformed so function elements can be resolved at runtime

* Match Global's styles prop to css prop behavior

* Update snapshots
@github-actions github-actions bot mentioned this pull request Nov 10, 2020
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

9 participants