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

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

Closed
MadLittleMods opened this issue Apr 25, 2015 · 10 comments

Comments

@MadLittleMods
Copy link

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
Copy link
Contributor

magsout commented Apr 25, 2015

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
Copy link
Author

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: postcss/postcss-mixins#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
Copy link
Owner

MoOx commented Apr 26, 2015

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 postcss/postcss-custom-properties#9 (comment)

@MadLittleMods
Copy link
Author

@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
Copy link
Owner

MoOx commented Apr 26, 2015

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

@MadLittleMods
Copy link
Author

@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.

  • handleRootVariables
  • handleRootVariableChanges
  • handleRootVariableDeclarations
  • handleAnyRootVariable
  • enhanceRootVariables
  • enhanceRootVariableSupport
  • enhanceRootVariableTransformation
  • completeRootVariableTransformation
  • rootVariableCompleteSubset
  • handleAllTopLevelVariables

@MoOx
Copy link
Owner

MoOx commented Apr 26, 2015

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
Copy link
Author

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

  • Removes :root level variable declaration limitation
  • Usage in At-rule @media, @support
  • Nesting support with descendant selector or physical position along with postcss-nested

@MoOx
Copy link
Owner

MoOx commented Apr 30, 2015

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

@MoOx
Copy link
Owner

MoOx commented May 5, 2015

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.

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

No branches or pull requests

3 participants