callstack / linaria

Zero-runtime CSS in JS library
https://linaria.dev
MIT License
11.49k stars 414 forks source link

Modifier classes #234

Open ai opened 5 years ago

ai commented 5 years ago

Do you want to request a feature or report a bug?

feature

What is the expected behavior?

To have the same modifiers syntax as in astroturf

const Button = styled('button')`
  color: black;
  border: 1px solid black;
  background-color: white;

  &.primary {
    color: blue;
    border: 1px solid blue;
  }

  &.color-green {
    color: green;
  }
`;

<Button primary color="green">

Compare to ${ } interpolation it keeps CSS syntax. CSS highlight and autocomplete works.

satya164 commented 5 years ago

@thymikee @zamotany thoughts?

thymikee commented 5 years ago

I think it introduces a level of indirection and complicates stuff. I'm not a fan.

ai commented 5 years ago

@thymikee is it OK to simplify a lot of user’s code but increase complexity in a framework. Is how all features ware made?

Why you are not a fan of it?

thymikee commented 5 years ago

I mean I'm not a fan of taking arbitrary props and converting them implicitly to be used by CSS. What about prop name clashes if we wrap custom components with styled()? But I must say that not having interpolations seems nice, especially when converting regular CSS to JS.

ai commented 5 years ago

What about prop name clashes if we wrap custom components with styled()?

Can you show a small example? I am thinking that I was worry about it too, but not sure that we are speaking about the same.

satya164 commented 5 years ago

For example, say:

const Wrapper = styled(MyComponent)`
  &.primary {
    color: blue;
    border: 1px solid blue;
  }
`;

<Wrapper primary />

The primary prop is also going to be passed to MyComponent unless we filter it out. But we don't know supported props, so not possible filter it out.

Wondering something like this is a good idea:

<Wrapper {...mx({ primary: true, color: 'green' })} />

Or maybe the user can use this package: https://npmjs.com/package/classnames

I'm not opposed to the idea, but probably need to think a bit more to avoid the edge cases.

thymikee commented 5 years ago

Was about to write the same, but Satya 🤖 was quicker!

ai commented 5 years ago

@satya164 why we don’t know about supported props?

We have styled object from CSS Modules and can look into classes. astroturf does the same to detect what properties should be used as style modifiers. https://github.com/4Catalyzer/astroturf/blob/master/src/runtime/styled.js#L23-L25

satya164 commented 5 years ago

@ai I was talking about the props that are accepted by MyComponent (in above case). We don't know if a prop is used only for styling or is also a regular prop. If the user uses a prop only for styling and later someone else adds that prop to the component without checking the callsites, it might change the behaviour.

It's also a problem with property interpolation though, so not specific to this approach. I'm thinking if there is a way to distinguish style only props. Maybe instead of primary, the user can write $primary, and we know for sure that it's a prop only used for styling, and we filter them out.

ai commented 5 years ago

@satya164

Is styles the list of styles applied to the component and astroturf keeps them in the bundle?

Yeap, styles is that object from CSS Modules { list: 'component_list_hb4h5h45' }. We need it anyway when we use styled().

I was talking about the props that are accepted by MyComponent (in above case).

We can have the agreement that if prop is defined by class in styled(), it will not be passed down. It is pretty expected behaviour. If you declare prop as a class in styles, it definitely stops to be a just regular prop.

satya164 commented 5 years ago

Also, what do you think about data- attributes? This is already supported:

const Box = styled.div`
  /* some css */

  &[data-primary] {
    border: 2px solid blue;
  }  

  &[data-color=green] {
    color: green;
  }
`;

<Box data-primary data-color="green" />
ai commented 5 years ago
  1. data- attributes will be DOM
  2. It requires more symbols
  3. It is also a little hacky. data- attributes were created for a different purpose. People will not love them, as result it will not be syntax sugar feature for linaria as it works for astroturf.
satya164 commented 5 years ago

Makes sense. We'll think about it a bit more then.

Any reason you don't like following:

<Box $primary $color="green" />

One extra character, but I prefer to be explicit rather than assuming which properties shouldn't be passed.

ai commented 5 years ago

$primary is OK if we will have a rational reason to not use styles object to filter props

satya164 commented 5 years ago

@ai currently linaria doesn't use css-modules, just normal css-loader, so we don't have a styles object. it can be implemented though. though the reason I prefer $primary is because it's explicit that it's for styling, and not a regular prop/attribute. but maybe that's unnecessary. I just need to think a bit more :D

ai commented 5 years ago

currently linaria doesn't use css-modules

How do you scope keyframes? Or what if I don’t want to scope class (:global() in CSS or injectGlobal in SC)?

Did I understand correctly that right now you collect all styles to the single file styles.css? How code splitting works?

satya164 commented 5 years ago

How do you scope keyframes? Or what if I don’t want to scope class

Currently, we use stylis as the parser (though planning to switch to postcss for few reasons) which handles scoped keyframes as well as :global.

Did I understand correctly that right now you collect all styles to the single file styles.css? How code splitting works?

We create separate CSS files for each JS file and add a require statement, which gets picked up by css loader. Code splitting works similar to when you import vanilla css files with mini-css-extract-plugin.

brandonkal commented 5 years ago

Here's an idea if you want to keep nesting rather than composing individual classnames that would work with linaria's styled approach of using CSS variables for interpolations:

const Button = styled.button`
  color: black;
  border: 1px solid black;
  background-color: white;

  &[props|primary] {
    color: blue;
    border: 1px solid blue;
  }

  &[props|color=green] {
    color: green;
  }
`;

<Button primary color="green">

Linaria would then transform it to this before running stylis:

const Button = styled.button`
  color: black;
  border: 1px solid black;
  background-color: white;

  &[style*="--a"] {
    color: blue;
    border: 1px solid blue;
  }

  &[style*="--b"] {
    color: green;
  }
`;

<Button primary color="green">

Linaria would then accept the props for primary and color and if they match what was specified, it would apply the appropriate styles:

<button class="b13mnax5" style="--a:1;">Primary Button</button>

Here the value 1 is not important, we are matching for the word "--a". This feels a little hacky. These really could be converted to classnames instead, applied based on state, but I am leaving it as an option to consider.

What I like: Using css namespaces rather than JS interpolation. It is small annoyance to type out a function every time: ${props => props.primary || 'fallback'}

brandonkal commented 5 years ago

I've added a comment to my RFC that specifically addresses this issue. I've included a lot of detail, but the essence of it is this: Rather than using an arbitrary prefix for style only props, instead specify in the CSS rule if a prop should not be forwarded. Proposed syntax is like this:

&[props|primary--] {}

-- suffix, do not pass prop to children. Props are passed to children by default (unless it is a styled DOM node where valid props are known). Simply include a -- to opt out of this for a specific prop.

ShanonJackson commented 5 years ago

Not a fan of the syntax especially in terms of static typing.

Maybe something like this...

const Button = styled.button`
      color: red;
`.variants({
       primary: css`color: blue`,
       secondary: css`color: white`,
       disabled: css`cursor: default`
});

const OneVariant = <Button variant={"primary"}/>
const MultiVariant = <Button variant={["primary", "disabled"]}/>

and for static typing (typescript)

.variants<"primary" | "secondary" | "disabled>({....
// which types variants as Record<T, CSS>
brandonkal commented 5 years ago

Yes, I have revised by thoughts on the syntax. This is what I am using for my ~60 component UI library and it works out quite well. It feels a lot closer to CSS syntax IMO and works with static typing:

interface ButtonProps {
  primary$?: boolean;
}

const Button = (p => styled.button<ButtonProps>`
  background: black;
  color: white;
  &${[p.primary$]} {
    background: blue;
    color: black;
    &:hover {
      border-color: red;
    }
  }
`)({} as ButtonProps);

This works much better for static typing than my comment above.

The only issue I have run into with my syntax is I cannot target the primary modifier in another component. This isn't so bad, as styles are self contained, but I did find I wanted to do this once.

Your approach is interesting, but I am not a fan of having to call the css function everywhere. Also, accepting an array feels very much like reimplementing classnames and you lose out on accepting an arbitrary prop that can be used for other logic (you'd have to do let isPrimary = props.variant.includes('primary') in your render functions rather than destructuring a prop).

Another downside to your approach is you can't target multiple variants, i.e. applying some styles when both primary and disabled are enabled.

ShanonJackson commented 5 years ago

There's an example there with multiple modifiers

brandonkal commented 5 years ago

No there is not. You misunderstood what I intended to convey. Take this css:

.button {
  background: pink;
}
.button__primary {
  background: green;
}
.button__disabled {
  cursor: default;
}
/* This */
.button__disabled.button__primary {
  color: red;
}
ShanonJackson commented 4 years ago

Thought it about this for awhile the solution would be this:

styled.div generates an object (not a string like css), therefore just add modifiers as statics on that object.

I.E

const Button = styled.button`
      color: red;
`.variants({
       primary: `color: blue`,
       secondary: `color: white`,
       disabled: `cursor: default`
});

const TargetDisabledButton = css`
    .${Button.disabled} {
           color: coralpink;
    }
`

Here's an example implementation to work around non-native syntax that works in the current version of linaria.

const Card = styled.div`
    color: red;
` ;

Card.variants = (obj) => Object.keys(obj).forEach((key) => (Card[key] = obj[key]));
Card.variants({
    get disabled() {
        return css`
            pointer-events: none;
            opacity: 0.4;
            user-select: none;
        `;
    },
});

// somewhere in a render and it works.
<Card className={Card.disabled}>HELLO</Card>

And the same code with hopefully a native implementation

const Card = styled.div`
    color: red;
`.variants({
     disabled: `
            pointer-events: none;
        opacity: 0.4;
            user-select: none;
      `
}) ;

<Card variants={[disabled && "disabled"]}/>

const OtherComponent = styled.div`
        .${Card.disabled} {
                color: blue;
        }
`

@satya164 @brandonkal i hope this satisfies your concerns for a modifier implementation

brandonkal commented 4 years ago

That is a creative solution but I am still not a fan. To provide some reasons:

interface ButtonProps {
  primary$?: boolean;
}

const Button = styled.button<ButtonProps>`
  color: white;
  &${[window.isUserLoggedIn]} {
    background: blue;
  }
`

That logic could of course be more complex than checking the truthiness of a window variable. The last suggestion would require repeating that logic everywhere Button is used.

I believe there should be a way to add static typing (i.e. accessing modifiers as if they were properties of a styled component (they are, but only after the linaria transform) to my suggested approach.

ShanonJackson commented 4 years ago

export const ButtonPrimary = csscolor: blue; export const ButtonSecondary = csscolor: blue; export const ButtonDisabeld = csscursor: default;

or this which i try to do more lately..

import { css } from 'linaria' const Button = styled.button color: red; ;

const ButtonPrimary = csscolor: blue; const ButtonSecondary = csscolor: blue; const ButtonDisabeld = csscursor: default; export const ButtonCss = { primary: ButtonPrimary, secondary: ButtonSecondary, disabled: ButtonDisabled }


the proposed way cleans up my code an insane amount.
brandonkal commented 4 years ago

The reason I say the object approach requires the css keyword is because no syntax highlighter or linter will accept a plain literal. That last example is just to illustrate that you can link state to style logic with the approach I suggested. The logic could be much more complex than just reading something off window in my simple example. The approach is inspired by LinkedIn's CSSBlocks if you wish to look into that.

Jayphen commented 4 years ago

Currently, we use stylis as the parser (though planning to switch to postcss for few reasons)

@satya164 What were those reasons?

I disabled stylis in my build, but haven't been able to get global styles working properly without it yet.

ShanonJackson commented 4 years ago

Linters and and code highlighters are just tooling; I'd rather rewrite the tooling based on ideal implementation than use a non-ideal implementation and avoid haivng to rewrite the tooling.

Syntax highlighting and linters will 100% accept a plain literal you just wont trigger it automatically on any literal you'll look to see if that literal looks like it belongs to styled component; Walk the AST.

EDIT: My point is not that my implementation is ideal; just that any implementation shouldn't be discarded just because tooling wont work anymore. Its unnecessary to have css tags there. I'm also more than happy to rewrite the tooling for the linaria community myself iv'e got strong experience with AST parsing

alexojegu commented 1 year ago

I have few experience with CSS and with Linaria, but this seems to work:

interface TestProps {
    $color: string;
    className?: string;
}

const Test = styled.a<TestProps>`
    text-decoration: none;
    &.foo {
        color: ${({ $color }) => $color};
    }
    &.bar {
        font-weight: 900;
    }
`;

<Test href="#" $color="red" className="foo">Text</Test>
<Test href="#" $color="blue" className={cx("foo", "bar")}>Text</Test>
<Test href="#" $color="red" className={cx("foo", "bar", css`
    background-color: blue;
`)}>Text</Test>

And this too:

const Test2 = styled(Test)`
    &.bar {
        font-size: 32px;
    }
`;

<Test2 href="#" $color="red" className={cx("foo", "bar", css`
    background-color: blue;
`)}>Text</Test2>

Yeah, I haven't tested it 100% and the DX with static typing is bad.

dreyks commented 1 year ago

is this still being considered as a possible built-in feature?

I'm using a webpack loader that I've made (not open-source for now) that runs before linaria loader and turns this code

const Component = styled('div')`
  &.foo {
    color: green;
  }

  &.bar-baz {
    background-color: red;
  }
`

into this code

const linariaAutopilotInterpolations0 = {
  'foo': css``,
  'bar-baz': css``
}

const LinariaAutopilotInternalComponent0 = styled('div')`
  &.${linariaAutopilotInterpolations0['foo']} {
    color: green;
  }

  &.${linariaAutopilotInterpolations0['bar-baz']} {
    background-color: red;
  }
`

const Component = getRuntimeNode(LinariaAutopilotInternalComponent0, linariaAutopilotInterpolations0)

where the getRuntimeNode returns a HOC that maps props into correct classnames

this supports all css features like nesting and does not require any new concepts to be introduced either for the consumers or the library

WDYT?

dpashk-figma commented 7 months ago

Just want to add my vote to the original proposal, I'll copy-paste it to reduce the up and down scrolling:

const Button = styled('button')`
  color: black;
  border: 1px solid black;
  background-color: white;

  &.primary {
    color: blue;
    border: 1px solid blue;
  }

  &.color-green {
    color: green;
  }
`;

<Button primary color="green">

I used astroturf extensively and was a big fan of the auto-generated modifier class names as illustrated in the original post. It's much more elegant and idiomatic than having a bunch of lines that look like this color: ${props => (props.primary ? 'tomato' : 'black')};. Besides the latter syntax looking bulky, it also doesn't let you syntactically group all the style rules that you want to apply if the "primary" modifier is set.

The OP also requires zero boilerplate vs the .variants({}) proposal above, for instance. You just write your styles in pretty much the same way you used to CSS stylesheets with semantic class names back in the day, except these semantic class names are set from component props. It really makes a difference in the developer experience.

dreyks commented 7 months ago

btw, I've made a "pre-loader" for linaria that makes it behave like astroturf by replacing modifier classes with css templates (at build time)

basically it turns this

import { styled } from '@linaria/react'

export const Container = styled('div')`
  background-color: rebeccapurple;

  &.something {
    width: 100%;
  }
`

into this

import { styled } from '@linaria/react'
import { css } from '@linaria/core'
import { getRuntimeNode } from '../../../runtime.js'

const linariaAutopilotInterpolations0 = {
  'something': css``
}

const LinariaAutopilotInternalComponent0 = styled('div')`
  background-color: rebeccapurple;

  &.${linariaAutopilotInterpolations0['something']} {
    width: 100%;
  }
`

export const Container = getRuntimeNode(LinariaAutopilotInternalComponent0, linariaAutopilotInterpolations0)

the getRuntimeNode is similar to what astroturf has

it works good, but it has some flaws because it meant to be a fast-and-dirty implementation, so its cuts some corners and I sadly don't have time atm to make it right. I'll try to check out wyw-in-js, maybe it'll be easier

dpashk-figma commented 7 months ago

@dreyks are you able to share your pre-loader code?

dreyks commented 7 months ago

yup, here it is https://gist.github.com/dreyks/36ffcd4734b56b8f0c32a78127925d83. can be used as a webpack loader, jest transform, or manually from a script. the main issue right now is that components created this way can't be re-styled as styled(Something) because the actual variable name (Something) affects linaria classname generation, and after going through the preloader the name gets changed to LinariaAutoPilotInternalComponent