MadLittleMods / postcss-css-variables

PostCSS plugin to transform CSS Custom Properties(CSS variables) syntax into a static representation
https://madlittlemods.github.io/postcss-css-variables/playground/
Other
536 stars 62 forks source link

Please clearly show in the README the limitation / bugs it can create #4

Closed MoOx closed 9 years ago

MoOx commented 9 years ago

Currently you use a title like

.root {
  --text-color: red;
}

.component {
  --text-color: blue;
}

.component .header {
  color: var(--text-color);
}

.component .text {
  --text-color: green;
  color: var(--text-color);
}

With the current html, this is completlly wrong

<div class="component">
  Black
  <div class="header">
    Blue
    <div class="text">
      Green
      <div class="header">Green</div>
    </div>
  </div>
</div>

See native result on firefox http://jsbin.com/tireneluxo/1/edit?html,css,output

Your output give a completly different result

http://jsbin.com/zacakilamo/1/edit?html,css,output

It's unexpected according to what I can read in the readme https://github.com/MadLittleMods/postcss-css-variables#using-cascading-variables-the-var-notation

Using Cascading Variables: the var() notation ...

  • No limitation on what scope CSS variables can be declared or used (:root or wherever)
    • Proper value substition based on explicit DOM/structure traversal

So it's totally unclear.

tunnckoCore commented 9 years ago

I dont see problem. http://jsbin.com/wewunusubu/2

.root {
  --text-color: red;
}

.component {
  --text-color: blue;
}

.component .header {
  color: var(--text-color);
}

.component .text {
  --text-color: green;
  color: var(--text-color);
}

this would be compiled to

.component .header {
  color: blue;
}

.component .text {
  color: green;
}

so <div class="header">Green</div> would be blue because the defined color in .component .header the problem here is other... this overriding. .component --text-color override .root --text-color

MoOx commented 9 years ago

the problem here is other... this overriding. .component --text-color override .root --text-color

Yes this is ONE problem.

The output you provide is completely not enough to say "No limitation".

And when I read again this quote, I see a something wrong:

Using Cascading Variables: the var() notation ...

  • No limitation on what scope CSS variables can be declared or used (:root or wherever)
    • Proper value substition based on explicit DOM/structure traversal

There is a bug + limitation with the current implementation.

MadLittleMods commented 9 years ago

Proper value substition based on explicit DOM/structure traversal

This is trying to convey: we look through the explicit structure of the rules and selectors in your CSS and resolve the value from that.

I understand that with real CSS variables, the true cascade structure would resolve the values you are expecting.

To get what you expect with postcss-css-variables, you need explicitly state this structure:

Input:

.root {
  --text-color: red;
}

.component {
  --text-color: blue;
}

.component .header {
  color: var(--text-color);
}

.component .text {
  --text-color: green;
  color: var(--text-color);

  /* This #1 */
  .header {
    color: var(--text-color);
  }
}

/* That #2 */
.component .text .header {
  color: var(--text-color);
}

Output:

.component .header {
  color: blue;
}

.component .text {
  color: green;

  /* This #1 */
  .header {
    color: green;
  }
}

/* That #2 */
.component .text .header {
  color: green;
}
MoOx commented 9 years ago

So I have to repeat some definition with more css ? Looks like you are not really adding value that much ;)

MadLittleMods commented 9 years ago

@MoOx I think what you are wanting would be in the minority of what people actually want/expect. I realize the CSS custom properties spec would make it your way which does make sense, but definitely not in terms of this plugin because no nesting structure was provided for us to work from.

If I was nesting a component over and over, I would expect it it look the same.


Maybe a better solution to your problem would be to use currentColor for any nested components.

/* ... */

.header .header {
    color: currrentColor;
}
MoOx commented 9 years ago

I think what you are wanting would be in the minority of what people actually want/expect.

You choose the syntax from w3c spec and you don't want to follow the spec ? Weird.

but definitely not in terms of this plugin because no nesting structure was provided for us to work from

So don't put link to w3c spec everywhere in the readme. You are harming people that are eager to use w3c spec compliant code.

currrentColor

color was just an example. If you put any other properties, this whould work as expected by the specs?

MadLittleMods commented 9 years ago

@MoOx I think the opening part of the readme explains pretty well and reiterates multiple times, that this can not perform exactly as the spec.

The spec is linked for reference so people can check out the syntax or compare to the actual functionality.


Because CSS custom properties are not widely supported. The use-case you bring up is not widely used today and in my opinion, is not a very good way to construct styles. To get the output you expect now, you would have to nest/descendant-selector anyway.

dead-claudia commented 8 years ago

@MadLittleMods Mind noting this spec deviation on the readme? Unlike some, I'm not as concerned about intentional deviations from spec (even Babel has a several with ES2015), but I'd find it much more comforting knowing them.

I think this stems from the fact this plugin makes some unsafe assumptions regarding the selectors, like above. As in with this CSS:

.root {
  --text-color: red;
}

.component {
  --text-color: blue;
}

.component .header {
  color: var(--text-color);
}

.component .text {
  --text-color: green;
  color: var(--text-color);
}

you'll get this for each one:

//- In Jade, for brevity
.component: Black text
.component: .header: Blue text
.component: .header: .text: Green text
.component: .header: .text: .header: Green text per spec, blue per this polyfill

The reason this is the case is because of selector order, and this plugin substitutes the color when it's not necessarily safe to do so. The result will end up like this:

.root {
  /* --text-color: red; */
}

.component {
  /* --text-color: blue; */
}

.component .header {
  color: /* var(--text-color) */ blue;
}

.component .text {
  /* --text-color: green; */
  color: /* var(--text-color) */ green;
}

/* The above, reduced */
.component .header { color: blue; }
.component .text { color: green; }

The browser knows that .component > .header > .text > .header will trigger the .component .text selector to change the --text-color variable to green and then use that variable's value in .component .header for the color property.

This plugin, on the other hand, does not. It has no ability to know anything about the HTML itself (especially if it's generated through JS), and thus it is prone to making unsafe assumptions unless it's ultra-conservative like postcss-custom-properties.


It would be appreciated if you note this in the README, though.

MadLittleMods commented 8 years ago

Hey @isiahmeadows,

Per your investigation and this issue, the deviation is not knowing the actual DOM structure and instead we work off the explicit structure assumed from your CSS selectors.

Feel free to make a PR to update the readme to make this point more clear :grinning:

Currently we have this:

We strive for the most complete transformation but we/no plugin can achieve true complete parity according to the specification because of the DOM cascade unknowns.

and this comment which definitely needs to be rewritten as I meant to say something like "assumed DOM structure from your explicit CSS selectors."

Proper value substitution based on explicit DOM/structure traversal