Media query with $until fails

[edwardhorsford]

I think I have a bug with the media queries.

The following will not work:

If I change to $from: tablet it's fine. Weirdly it doesn't cause a compiler issue -but the css doesn't seem to be output.

When trying with extend I get an error with the extend - but I think it's related to this mixin.

[NickColley]

Hey @edwardhorsford,

I've tried to reproduce your issue here:

https://govuk-frontend-issue-1674.glitch.me

html::before {
  position: absolute;
  background: white;
  padding: 5px;
  font-family: sans-serif;
  @extend %until-mobile-only;
  @extend %mobile-only;
}

%until-mobile-only {
  @include govuk-media-query($until: mobile) {
    content: 'govuk-media-query($until: mobile)';
  }
}

%mobile-only {
  @include govuk-media-query(mobile) {
    content: 'govuk-media-query(mobile)';
  }
}

And it seems to output as expected, could you provide some steps to reproduce the issue you're running into please?

Update: It fails only when $govuk-is-ie8 is set to true, see below for full comments.

[edwardhorsford]

@NickColley I wonder if this might be a compiler issue / incompatibility. Your exact snippet above fails for me - we're using sass-rails

My snippet also failed for @hannalaakso - so perhaps she can demo.

In the end I've worked around it.

[kr8n3r]

above snippet will fail to compile in sass-rails (5.0.6) but will compile in sass-rails 6.0.0 which is a wrapper for sassc-rails

moving to sassc-rails gem comes with it's own set of issues documented here #1350

[hannalaakso]

I can replicate this in the latest version of the kit (but not in FE review app for instance) with:

%display-mobile-only {
  @include govuk-media-query($until: mobile) {
    display: none;
  }
}

.app-sidebar-hr {
  @extend %display-mobile-only;
}

[NickColley]

OK so this is a bit of a twisty one but I think I've gotten to the bottom of it.

What is happening is that when the Internet Explorer 8 version of the application.css is built. It sets the $govuk-is-ie8 variable to true.

This then sets the $mq-responsive variable to false.

mq is the library we use to generate our media queries. The responsive variable allows you generate a 'desktop only' build for older browsers, it does this by figuring out which media queries would be shown at 'desktop only' and removing ones on 'mobile'.

This means that when the mixin runs, it outputs nothing, which means your placeholder %display-mobile-only class fails to be extended because it's empty.

So you'll see this error message:

Error: "html::before" failed to @extend "%until-mobile-only".
       The selector "%until-mobile-only" was not found.
       Use "@extend %until-mobile-only !optional" if the extend should be able to fail.
        on line 26 of app/assets/sass/application-ie8.scss
>>     @extend %until-mobile-only;

I think the differences between compilers/wrappers is perhaps how they handle errors in this case.

I have updated my example to include the $govuk-is-ie8: true option and now it fails: https://glitch.com/edit/#!/govuk-frontend-issue-1674?path=src/styles/all.scss:2:0

I'm have also raised an issue with sass-mq to see if we can make this better without work arounds: sass-mq/sass-mq#130

Workaround

• avoid using placeholders with only govuk-media-query($until) within them, for example:

%display-mobile-only {
  @include govuk-media-query($until: mobile) {
    display: none;
  }
  /* noop */
}

or (recommended)

• use the !optional flag when extending, for example:

%display-mobile-only {
  @include govuk-media-query($until: mobile) {
    display: none;
  }
}

.app-sidebar-hr {
   // using !optional to work around the placeholder being empty in Internet Explorer 8 builds.
  @extend %display-mobile-only !optional;
}

I'm going to close this for now as I think this is the best I can offer at this point in time but if there's something I've missed or a better way to work around this feel free to re-open, thanks :)

[edwardhorsford]

Thanks for investigating this @NickColley!