styled-components / styled-components

Visual primitives for the component age. Use the best bits of ES6 and CSS to style your apps without stress 💅
https://styled-components.com
MIT License
40.45k stars 2.5k forks source link

Usage of css helper function #1178

Closed Jmeyering closed 6 years ago

Jmeyering commented 7 years ago

Based on a conversation over at https://github.com/styled-components/vim-styled-components/pull/30 I wanted to talk a bit about the purpose of the css helper mixin.

The current documentation states

css A helper function to generate CSS from a template literal with interpolations. You need to use this if you return a template literal with interpolations inside an interpolation. (This is due to how tagged template literals work)

If you're just returning a normal string you do not need to use this.

Based on the conversation generated in the above PR it seems this description is no longer accurate? And the css helper is intended to be used whenever the user breaks out of an interpolation.

This feels like a sledgehammer solution to the problem when education would be more beneficial.

I feel that the current documentation does a very good job of describing the appropriate use case for the css helper and that the informed developer knows when it's needed vs. when returning a plain string is acceptable.

mxstbr commented 7 years ago

You can indeed return a normal string, but it breaks syntax highlighting in all editors and if a developer comes along and expects an interpolation to work they're going to have a bad time:

import { lighten } from 'polished';

const Button = styled.button`
  ${props => props.primary && `
    color: ${lighten('#000')}; // This returns an object so it won't work
    background: ${props => props.bgColor}; // This won't work either, it's not executed
  `}
`

That's why we generally recommend using css anywhere. The reason we might enforce this in the future is because otherwise we cannot do any pre-runtime analysis on the code, meaning our Babel plugin can't minify your code, it can't pre-parse your code etc. (note, it's a big might)

I agree that the documentation here should be updated to reflect that we recommend folks generally to use it. /cc @philpl for his thoughts on this

kitten commented 7 years ago

Yep, we haven’t added a section for it to the docs yet! Sorry for that 😭

It will become even more important in v3, since well need the tag to hook on to the css then

Edit Regarding what the API docs state, that is still accurate, but not in line with tooling. Since the API docs purely state functionality and usage regardless of tools, it doesn’t mention the part where syntax highlighters need it

Jmeyering commented 7 years ago

I guess if there is a solid reason for it I understand but if it's not doing anything to a plain string i'd rather not bother with it personally.

On Sep 26, 2017 1:23 PM, "Phil Plückthun" notifications@github.com wrote:

Yep, we haven’t added a section for it to the docs yet! Sorry for that 😭

It will become even more important in v3, since well need the tag to hook on to the css then

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/styled-components/styled-components/issues/1178#issuecomment-332290009, or mute the thread https://github.com/notifications/unsubscribe-auth/AE7WCyArspWd-M-UADb-EA-jAXnBzfN6ks5smUEQgaJpZM4PkTBh .

mxstbr commented 7 years ago

if it's not doing anything to a plain string

Again, the main library doesn't but the Babel plugin does. It minifies and pre-processes your styles, but if you just put a string there we have no idea that it's CSS! That's where the css helper comes in, if it's tagged with css we know it's CSS and can minify and preprocess it.

Jmeyering commented 7 years ago

How does this affect mixins which internally use the css helper? I'm thinking specifically of the media template and other mixins structured in the same way.

https://www.styled-components.com/docs/advanced#media-templates

Jmeyering commented 7 years ago

@mxstbr I have a feeling the template pattern will also break static analysis? If so are there alternative solutions to this use case. I'm asking because were about to begin 3 new projects and would like to future proof our implementation as much as possible.

kitten commented 7 years ago

You are on the right track here. Unfortunately we won’t be targeting custom tagged template literals. Maybe we will at some point but for now it seems unlikely.

A future proof implementation would involve turning the input for such a media helper into: media.large(css``) for instance

I’m sure you could also work on a codemod when the time comes. We are just a couple of people with a lot of foundational work to be done until more can start to help and accelerate work on v3

mxstbr commented 6 years ago

Closing since there's nothing actionable for us to do here, thanks for forcing us to clarify @Jmeyering!