MoOx / postcss-cssnext

`postcss-cssnext` has been deprecated in favor of `postcss-preset-env`.
https://moox.io/blog/deprecating-cssnext/
MIT License
5.3k stars 188 forks source link

var declared in media query should pull in properties that use/reference that var #99

Closed MadLittleMods closed 9 years ago

MadLittleMods commented 9 years ago

Custom properties are ordinary properties, so they can be declared on any element, are resolved with the normal inheritance and cascade rules, can be made conditional with @media and other conditional rules, can be used in HTML’s style attribute, can be read or set using the CSSOM, etc.

from W3C Working Draft: CSS Custom Properties for Cascading Variables Module Level 1

:root {
    --width: 100px;
}

.box {
    width: var(--width);
}

@media (max-width: 1000px) {
    :root {
        --width: 200px;
    }
}

I expect or should result in:

.box {
    width: 100px;
}

@media (max-width: 1000px) {
    .box {
        width: 200px;
    }
}

Is this a correct understanding?

Playing around in the cssnext playground, I am getting different results.

I understand this could get a little tricky in a situation like described here.

magsout commented 9 years ago

https://github.com/postcss/postcss-custom-properties/blob/master/README.md

The transformation is not complete. It currently just aims to provide a future-proof way of using a limited subset (to top-level :root selector) of the features provided by native CSS custom properties.
Read #1 & #9 to know why this limitation exists.

MadLittleMods commented 9 years ago

Put it behind a flag

I think the limitation is valid for being implemented as a default. I think if this feature would be best suited behind a flag. It would cover/solve the actual use cases that I have seen throughout the issues you linked, issues on the Sass repo, etc.

Unknown DOM Structure

For starters, the flag would only enable :root level variable declarations to avoid the "magic" needed to cover the whole spectrum of DOM structure.

Cascade

specificity/cascade issues - moving/generating rules is always going to hit the problem where earlier styles are unintentionally overridden. ... These kinds of divergences are unavoidable when you inject rules or move them around. All the media query packer plugins have the same caveat.

from #9

Don't try to move around pack/group media queries. Some simple option presets could solve the functionality.

  1. Move everything to where the media query re-declaration is
  2. Create a specific media query below each rule

Example

Input:

:root {
    --foo-width: 15vw;
    --bar-width: 150px;
}

.box-foo {
    width: var(--foo-width);
}

.box-bar {
    width: var(--bar-width);
}

.box-qux {
    width: calc(400px - var(--bar-width));
}

@media (max-width: 800px) {
    :root {
        --foo-width: 25vw;
        --bar-width: 300px;
    }
}

Output with preset #1 (move everything to where the media query re-declaration is):

.box-foo {
    width: 15vw;
}

.box-bar {
    width: 150px;
}

.box-qux {
    width: calc(400px - 150px);
}

@media (max-width: 800px) {
    .box-foo {
        width: 25vw;
    }

    .box-bar {
        width: 300px;
    }

    .box-qux {
        width: calc(400px - 300px);
    }
}

Output with preset #2 (create a specific media query below each rule):

.box-foo {
    width: 15vw;
}
@media (max-width: 800px) {
    .box-foo {
        width: 25vw;
    }
}

.box-bar {
    width: 150px;
}
@media (max-width: 800px) {
    .box-bar {
        width: 300px;
    }
}

.box-qux {
    width: calc(400px - 150px);
}
@media (max-width: 800px) {
    .box-qux {
        width: calc(400px - 300px);
    }
}

There is no better solution/alternative

This use case could be accomplished with a robust enough mixin system.

But right now, postcss-mixins doesn't support @content style mixins or passing arguments from a mixin to a content block. See: https://github.com/postcss/postcss-mixins/issues/8 for progress on this feature.

Combined with postcss-nested, we could accomplish the same as the snippet below in with PostCSS.

Note: non-valid Sass. until this issue is resolved

$media-variable-map: (
    default: (
        foo-width: 15vw,
        bar-width: 150px
    ),
    800px: (
        foo-width: 25vw,
        bar-wdith: 300px
    )
);

@mixin access-variables {
    @each $width, $variable-map in $media-variable-map {
        @if($width != default) {
            @media (max-width: #{$width}) {
                @content($variable-map)
            }
        }
        @else {
            @content($variable-map)
        }
    }
}

.box-foo {
    @include access-variables using($variable-map) {
        width: map-get($variable-map, foo-width);
    }
}

.box-bar {
    @include access-variables using($variable-map) {
        width: map-get($variable-map, bar-width);
    }
}

.box-qux {
    @include access-variables using($variable-map) {
        width: calc(400px - map-get($variable-map, bar-width));
    }
}
MoOx commented 9 years ago

I think the example in #9 is pretty clear about the huge issue with the first solution (inject after each :root definition).

<div class="One Two">Text</div>

Input:

:root { --fontSize: 2em; }
.One { font-size: var(--fontSize); }
.Two { font-size: 6em; }

@media (min-width: 64em) {
  :root { --font-size: 3em; }
}

Output (notice One now overrides Two, which it would not with a native solution):

.One { font-size: 2em; }
.Two { font-size: 6em; }

@media (min-width: 64em) {
  .One { font-size: 3em; }
}

But I've to admit, with my brains that is just waking up, the second solution seems to be not that bad. I've added a comment here about it https://github.com/postcss/postcss-custom-properties/issues/9#issuecomment-96322963

MadLittleMods commented 9 years ago

@MoOx I think your example/use-case is outside of the scope of this feature in terms of a reason to shoot down this feature as a whole(not saying you are, but others have in previous issues) but a completely valid case for why it may be best to put it behind a flag. The cascade solution #2 proposed earlier does solve this problem as you mentioned though.

While #2 clearly seems better, I recommend implementing both just in case someone is inclined to use it and doesn't have a situation like you describe. The default should be #2 to avoid failure as much as possible. Your kind of example/situation pops up frequently with BEM but not everyone uses that methodology and may want their CSS output to be like #1 for some unforeseen reason.

MoOx commented 9 years ago

I might start to work on solution #2 soon. Any idea on the name of the option to enable this ? handleRootDeclarationsInMediaQueries (lol)?

MadLittleMods commented 9 years ago

@MoOx

Note: I suppose this should probably work with @support directives as well.

Please make this flag available to the Node.js API.

Flag name suggestions

Since it is a bool, maybe prepend a should in front of some of these.

MoOx commented 9 years ago

Good point for @support and some others @ directives (@page?).

What about enableUnsafeTransformation (enableDangerousTransformation?). Or maybe just unsafe with a clear doc on what it will enable.

MadLittleMods commented 9 years ago

Just released the postcss-css-variables plugin to cover the use case this issue brings up.

MoOx commented 9 years ago

As said on gitter we should keep the effort on postcss-custom-properties plugin by incorporating safe transformation.

MoOx commented 9 years ago

I just take a deeper look at your plugin and opened too many issues that you won't be able to resolve to decide (I know because i tried to in the past) to not use it sorry. I will open another issue for :root in media queries.