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 emotion to UI #17793

Merged
merged 27 commits into from
Sep 15, 2017
Merged

Add emotion to UI #17793

merged 27 commits into from
Sep 15, 2017

Conversation

sndrs
Copy link
Member

@sndrs sndrs commented Sep 13, 2017

What does this change?

  • adds emotion to UI
  • removes all the SVG data-block stuff

any CSS which cannot be handled by styletron can now be managed like this:

<Logo
    css={`
        width: 250px;
        & .guardian {
            @supports (fill: hotpink) {
                fill: ${colour.brandBlue};
            }
        }
    `}
/>

in a subsequent PR, i'll update linting of *.js.scss files to disallow css that styletron won't be able to handle, and update the wiki.

What is the value of this and can you measure success?

styletron does not handle more complex CSS, for example combinator selectors. this adds emotion, along the lines of #17761, which does, but at the cost of heavier CSS.

it's much smaller than styled-jsx (2kb gzipped), and writing the code feels more natural. it's the result of unsuccessful but extensive effort to create an api more aligned with current way, but stops short of it to see how this is used.

@sndrs sndrs changed the title Ui/expensive style emotion Add emotion to UI Sep 13, 2017
@PRBuilds
Copy link

PRbuilds results:

Screenshots
desktop.pngtablet.pngmobile.pngwide.png

Exceptions (0)
thrown-exceptions.js

A11y validation (3)
a11y-report.txt

Apache Benchmark Load Testing
loadtesting.txt

--automated message

@gtrufitt
Copy link
Contributor

What's the general feeling of the team with styetron syntax vs something like this? I'd prefer writing like this tbh, but only because it still feels more familiar

@SiAdcock
Copy link
Contributor

SiAdcock commented Sep 13, 2017

@gtrufitt

I like this syntax as, again, it feels familiar and it keeps your styles close to your component markup.

There's a couple of possible downsides that I can think of:

  1. Too much visual clutter (imagine a component comprised of four or five elements, each with their own styles defined inline)
  2. No syntax highlighting or autocompletion (There's a styled-components plugin for IntelliJ, but I haven't found a plugin for emotion yet)

But as with any developer experience discussion, opinions are rife and the only way to "prove" whether hypothetical problems are actual problems is to test it out on developers.

@GHaberis
Copy link
Contributor

GHaberis commented Sep 13, 2017

I'd be happy to use this emotion syntax and agree that the syntax looks familiar enough to a JS developer.

I wonder how intuitive it'd be for someone whose used to writing SCSS/CSS but doesn't have a JS background? Like @SiAdcock said it's a hypothetical problem we're anticipating. Perhaps clear documentation/examples as guidance could get people up to speed with this approach quick enough? It's not a radical departure.

I do like the Styletron approach of importing named style rules from a scss file, primarily because of neatness and lack of visual clutter within the JSX.

Having named style rules also means reusing rules across elements is straight forward enough. Yes you could get around the reuse of rules in emotion by assigning the template-string to a named variable and assigning that to the css attribute, but a potential downside here is you'll end up with some rules defined inline and others not.

@sndrs
Copy link
Member Author

sndrs commented Sep 13, 2017

yeah i like the look of this. be v interesting to see what people go for. if we dropped styletron for just this (to settle on one api), even though we'd lose the tiny css, we'd still be at around 10% of current state, which is pretty good.

@sndrs
Copy link
Member Author

sndrs commented Sep 13, 2017

@SiAdcock one big thing in styled-components favour is the ecosystem, for sure

],
test: /\.js\.scss$/,
exclude: /node_modules/,
use: [{ loader: 'styletron-loader' }, sassLoader],
Copy link
Contributor

Choose a reason for hiding this comment

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

We could shorten this to :

use: ['styletron-loader', sassLoader],

props,
[
styletron.getStylesheetsHtml(),
`<style expensive-css>${extractCritical(body).css}</style>`,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the expensive-css attribute for? Is it simply flagging up this style element for developers / archaeologists?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah exactly that

Copy link
Contributor

@SiAdcock SiAdcock left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@NataliaLKB NataliaLKB left a comment

Choose a reason for hiding this comment

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

lgtm 💯

…ve-style-emotion

# Conflicts:
#	ui/src/.flowconfig
@sndrs sndrs merged commit 0ad5fa3 into master Sep 15, 2017
@sndrs sndrs deleted the ui/expensive-style-emotion branch September 15, 2017 11:06
@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @sndrs 16 minutes and 25 seconds ago)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants