Add ClassNames component #828

emmatown · 2018-08-29T23:29:03Z

What:
Add ~~Style~~ ClassNames component.

css and cx work just like the exports from emotion except they work with emotion 10's SSR and the options that can be provided via context.

It also accepts the theme.

<ClassNames>
  {({ css, cx, theme }) => {
    let secondClassButItsInsertedFirst = css`
      color: green;
    `
    let firstClassButItsInsertedSecond = css`
      color: hotpink;
    `

    return (
      <div
        className={cx(
          firstClassButItsInsertedSecond,
          'some-other-class',
          secondClassButItsInsertedFirst
        )}
      />
    )
  }}
</ClassNames>

Why:

To account for cases when components accept props like wrapperClassName and etc.

How:

Checklist:

Documentation (gonna add to Emotion 10 docs #799 after Reimplement vanilla emotion, babel-plugin-emotion with new utils and change react-emotion tests to new styled #824 is merged)
Tests
Code complete

I'm not totally sure about the API, the name of the component and whether it should be in the core package. I'd love to hear people's thoughts on it!!

Note: This is based on #824 not master.

Andarist · 2018-08-30T09:03:57Z

next-packages/core/src/style.js

+        break
+      case 'function':
+        if (process.env.NODE_ENV !== 'production') {
+          console.error(


is this needed? it's already "next major version of Emotion" :D

Nope, I made this a while ago and copied classnames from v9.

Andarist · 2018-08-30T09:06:28Z

next-packages/core/src/style.js

+  | { [key: string]: boolean }
+  | Array<ClassNameArg>
+
+let classnames = (args: Array<ClassNameArg>): string => {


can u explain more why this is duplicated? I suspect it's like a global caching registry issue, but couldnt this share the implementation with original cx, css & merge?

I’m just lazy, I think I’ll move it and merge into @emotion/utils. There’s no point in sharing css and cx since they need slightly different things in here and in create-emotion.

emmatown · 2018-08-30T09:11:06Z

Just FYI, I’m mainly looking for a review of the API rather than code review.

Andarist · 2018-08-30T11:25:19Z

Could you explain a little bit more why this is needed for SSR? I'm not too familiar with its intricacies

emmatown · 2018-08-30T11:30:20Z

Because there needs to be somewhere to render the style tags and the exact styles that need to be rendered there must be known when the style tag is being rendered, that's not possible if using something like css from emotion.(by SSR, I'm referring to emotion 10's automatic SSR)

Andarist · 2018-08-30T11:33:52Z

that's not possible if using something like css from emotion

Why? Isnt the css call synchronous anyway?

emmatown · 2018-08-30T11:41:42Z

Let's say you have this component, how is it possible to do automatic SSR?

let SomeComponent = () => (
  <div
    className={css`
      color: hotpink;
    `}
  />
)

emmatown · 2018-08-30T11:44:49Z

Also, I just want to repeat this, this is only intended for cases when components accept props like wrapperClassName and etc.

Andarist · 2018-08-30T20:15:21Z

Ok, I get now how it's impossible for auto-SSR.

The API looks good, although it slightly shows that even though current approach brings easier SSR to the table it also has its drawbacks such as having to know that sometimes <Style/> has to be used.

Also - not sure what's the best name for this component, maybe <ServerStyle/>?

emmatown · 2018-08-30T20:45:51Z

This isn't just for auto-SSR though, it's also for ~~providing~~ consuming the cache via context so that things like working in iframes, getting the theme and etc. work.

The ServerStyle name doesn't really make sense since it's not just for SSR.

Andarist · 2018-08-30T20:57:16Z

This isn't just for auto-SSR though, it's also for providing the cache via context so that things like working in iframes, getting the theme and etc. work.

Is this meant to be a replacement for previously presented APIs? You've already demoed i.e. rendering to iframes etc.

Sorry for asking so many questions, I'm not too familiar with v10 APIs - lacking a "big picture" and how existing pieces fit together.

emmatown · 2018-08-30T21:00:36Z

No, this isn't meant to be a replacement for the css prop or styled, it's meant for for the small number of cases that they can't solve.

Sorry, when I said providing the cache via context I meant consuming it via context.

Andarist · 2018-08-31T07:24:05Z

Ok, I think I get it now 👍 The only confusing part for me is the name of the component - for a css-in-js a component named <Style/> is rather confusing, like - it doesn't tell me much what it exactly do. A name like <Emotion/> (although not descriptive) would tell me more about what I can do with it. This might be though solely because I'm already accustomed to what emotion package provides.

From what I get it seems that we are going to have now 2 context consumer types - one holds registry, stylis, sheet etc and accounts for preferred way of writing "idiomatic" emotion (it doesnt provide things like css, because those are discouraged in react setting) and we have this other consumer - <Style/> which provides access to those "discouraged" helpers. Did I get this right?

emmatown · 2018-08-31T08:16:50Z

Yeah, that's mostly correct though this isn't very different from the other consumers, they all work in the same way, they just provide different APIs to insert styles. This API is just less convenient but more flexible.

I just thought of another name, ClassNames, any thoughts on that?

Andarist · 2018-08-31T09:22:37Z

I just thought of another name, ClassNames, any thoughts on that?

I like it better 👍

tkh44 · 2018-08-31T19:41:23Z

I like this name better.

I just thought of this, but what if it was just called Emotion

emmatown · 2018-08-31T22:55:25Z

@Andarist suggested that too, I feel like it doesn’t convey much meaning about what it does though.

Ailrun · 2018-08-31T22:58:26Z

For me, it's also hard to understand. How about SerialStyles or something like that?

emmatown · 2018-08-31T23:52:39Z

I like ClassNames because it conveys that it’s about class names since what it does is let you get a class name from some styles and merge together class names.

I don’t really get what SerialStyles means and how it would convey what this component does

Ailrun · 2018-08-31T23:55:52Z

Oh, I thought about css props part only. With cx props, what you said make sense.

but still... does ClassNames really convey the differences of css props from original css function?

Ailrun · 2018-09-01T00:02:33Z

css props is different original css, so the component name should convey the differences, shouldn't it?

emmatown · 2018-09-01T00:03:14Z

There isn’t really a difference between the css function in this component and the css function in v9 except for the implementation.

emmatown · 2018-09-01T00:04:04Z

Are you talking about the css prop in emotion 10 or the css function in this component?

Ailrun · 2018-09-01T00:04:51Z

It's different in SSR, isn't it?

emmatown · 2018-09-01T00:06:22Z

Kind of but the usage of it is the same which is the part that people care about

Ailrun · 2018-09-01T00:08:20Z

I mean the difference between css function from @emotion/core and from props of this component.

Ailrun · 2018-09-01T00:14:32Z

(I believe) I know interfaces of those functions are same, but if it work differently in some sense, it should convey those differences as a name, shouldn't it?

emmatown · 2018-09-01T00:18:10Z

If it works the same from the user’s perspective, why should they have to learn a new name for something that does the same thing?

emmatown · 2018-09-01T00:20:15Z

Oh sorry, didn’t see your comment about css from @emotion/core, yep that makes sense, got any name suggestions for it then?

Ailrun · 2018-09-01T02:31:03Z

Actually, that's where my SerialStyle comes from, since I thought those difference should be reflected in the name of component, not a name of actual props.
My reasoning is like following.

If we have different name for css function from @emotion/core and the css function for the children of this component, refactoring codes those use css function becomes harder.
But IMO, since there are differences, those difference should be reflected somewhere.
Where would be appropriate for this? I cannot think other than component name.

But what you said in #828 (comment) makes sense, so I need to think more about this.

However, if those refactoring cannot make sense from the first stage, then renaming css function for children of this component could be an option.

emmatown · 2018-09-01T02:51:07Z

If we have different name for css function from @emotion/core and the css function for the children of this component, refactoring codes those use css function becomes harder.

css from @emotion/core and the css function for the children of this component return totally different thing things though, the former returns a description of the styles while the latter returns a class name.

Ailrun · 2018-09-01T02:59:52Z

Oh, yes, then we don't need to worry about refactoring. Then instead of concerning about new name for component, giving appropriate name for css in this component would be better. Thank you for enlightening.

TrySound · 2018-09-13T09:29:22Z

Guys, there is a better solution than wrapping every component with another one render prop.

We can have the same api but with some requirements about where functions may be called.

import * as React from 'react';
import { Provider, css } from '@emotion/contextual';

export const Component = () => (
  <Provider>
    <div className={css({ width: 100, height: 100, background: '#000' })}>
      Content
    </div>
  </Provider>
)

The point here is that since 16.5 we are able to consume context with Context.unstable_read, which works in render phase. This api actually doesn't work with ssr yet (that's why it's unstable), but we already can start experiments on client side.

Ref facebook/react#13139

emmatown · 2018-09-13T10:09:22Z

This component doesn’t only need context though, it also needs to render elements.

pksunkara · 2018-09-20T09:03:56Z

This is needed for emotion to work with /~https://github.com/reactjs/react-modal. Check this commit in @storybooks.

codecov · 2018-09-27T09:02:54Z

Codecov Report

Merging #828 into reimplement-vanilla-emotion will decrease coverage by 0.77%.
The diff coverage is 72.72%.

Impacted Files	Coverage Δ
packages/core/src/context.js	`100% <100%> (ø)`	⬆️
packages/core/src/class-names.js	`72.22% <72.22%> (ø)`

emmatown · 2018-09-27T23:08:53Z

I'm gonna merge this into #824 because I want to try some stuff out. If anyone has any comments on it, you can still leave them here though.

Add Style component

3b05125

emmatown requested review from Ailrun, Andarist, ChristopherBiscardi and tkh44 August 29, 2018 23:29

Change error message

26e7460

Andarist reviewed Aug 30, 2018

View reviewed changes

Rename to ClassNames

9f40a14

emmatown changed the title ~~Add Style component~~ Add ClassNames component Aug 31, 2018

Fix a type

7882699

emmatown added 4 commits September 27, 2018 18:45

Merge branch 'reimplement-vanilla-emotion' into style-component

74cc3cc

Fix it mostly

99fd29b

Merge branch 'reimplement-vanilla-emotion' into style-component

9f8b62a

Format

ecfb58e

Fix flow error

71346ff

emmatown merged commit 915fff8 into reimplement-vanilla-emotion Sep 27, 2018

emmatown deleted the style-component branch September 27, 2018 23:09

Add ClassNames component #828

Add ClassNames component #828

Conversation

emmatown commented Aug 29, 2018 • edited Loading

Andarist Aug 30, 2018

Choose a reason for hiding this comment

emmatown Aug 30, 2018

Choose a reason for hiding this comment

Andarist Aug 30, 2018

Choose a reason for hiding this comment

emmatown Aug 30, 2018

Choose a reason for hiding this comment

emmatown commented Aug 30, 2018 • edited Loading

Andarist commented Aug 30, 2018

emmatown commented Aug 30, 2018

Andarist commented Aug 30, 2018

emmatown commented Aug 30, 2018

emmatown commented Aug 30, 2018

Andarist commented Aug 30, 2018

emmatown commented Aug 30, 2018 • edited Loading

Andarist commented Aug 30, 2018

emmatown commented Aug 30, 2018

Andarist commented Aug 31, 2018

emmatown commented Aug 31, 2018

Andarist commented Aug 31, 2018

tkh44 commented Aug 31, 2018

emmatown commented Aug 31, 2018

Ailrun commented Aug 31, 2018

emmatown commented Aug 31, 2018

Ailrun commented Aug 31, 2018 • edited Loading

Ailrun commented Sep 1, 2018

emmatown commented Sep 1, 2018

emmatown commented Sep 1, 2018

Ailrun commented Sep 1, 2018

emmatown commented Sep 1, 2018

Ailrun commented Sep 1, 2018

Ailrun commented Sep 1, 2018 • edited Loading

emmatown commented Sep 1, 2018

emmatown commented Sep 1, 2018 • edited Loading

Ailrun commented Sep 1, 2018 • edited Loading

emmatown commented Sep 1, 2018

Ailrun commented Sep 1, 2018

TrySound commented Sep 13, 2018

emmatown commented Sep 13, 2018

pksunkara commented Sep 20, 2018

codecov bot commented Sep 27, 2018 • edited Loading

Codecov Report

emmatown commented Sep 27, 2018

emmatown commented Aug 29, 2018 •

edited

Loading

emmatown commented Aug 30, 2018 •

edited

Loading

emmatown commented Aug 30, 2018 •

edited

Loading

Ailrun commented Aug 31, 2018 •

edited

Loading

Ailrun commented Sep 1, 2018 •

edited

Loading

emmatown commented Sep 1, 2018 •

edited

Loading

Ailrun commented Sep 1, 2018 •

edited

Loading

codecov bot commented Sep 27, 2018 •

edited

Loading