Wrap sass-mq as govuk-media-query #763
Conversation
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-763
| @@ -130,7 +130,7 @@ | |||
| /// @access public | |||
|
|
|||
| @mixin govuk-link-print-friendly { | |||
| @include mq($media-type: print) { | |||
| @include govuk-media-query($media-type: print) { | |||
There was a problem hiding this comment.
+1 for not using single letter variables for this new mixin
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-763
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-763
| @@ -13,10 +21,9 @@ $mq-show-breakpoints: (); | |||
| // When building a stylesheet for IE8, set $mq-responsive to false in order to | |||
| // 'rasterize' any media queries. | |||
|
|
|||
| $mq-responsive: true; | |||
There was a problem hiding this comment.
Moving this because the way Sass variable scoping works means that unless $mq-responsive is defined before this point, the $mq-responsive declaration only works within the scope of the @if – which caused a lot of headaches in the tests.
|
|
||
| // ========================================================= | ||
| // Wrangle sass-mq config... | ||
| // ========================================================= | ||
|
|
||
| // Pass our breakpoints and static breakpoint definitions through to sass-mq. | ||
| $mq-breakpoints: if(variable-exists(govuk-breakpoints), $govuk-breakpoints, ()); |
There was a problem hiding this comment.
Because we do this configuration 'at import time', any changes to $govuk-is-ie8, $govuk-breakpoints, $govuk-ie8-breakpoint made after the import has been done will not have any effect on govuk-media-query. We could move this wrangling into the govuk-media-query mixin, but it complicates the way we pass arguments somewhat, and relies on us being able to pass that config to mq which is an undocumented feature, and I can't think of any valid use cases where you'd want to change that config mid-way through your codebase.
Thoughts?
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-763
|
one think the abstraction is missing is adding new media-queries (somewhere up to desktop where we want them to rasterise for ie8) you can still ad them via |
|
Is that something we think people actually want to use? I don't think we need to expose everything It's still possible to override |
|
true. people might not do it that way / know of it. most likely will just add it to the breakpoints map |
|
One thing to consider is that if in the future we wanted to move away from using sass-mq we'd have to implement everything we've exposed as part of our API – if we implement every feature that mq offers, we're committing to providing those features or we'd be forced to introduce a breaking change to remove them. |
|
@igloosi did you mean to mark that as 'request changes'? |
‘Abstract away’ sass-mq so that we don’t expose it as part of our public API, which means we’d be able to remove it in the future if we wanted to roll our own or move to a different vendored solution.
kr8n3r
dismissed
their stale review
no changes to code needed, just changelog needs adding
govuk-design-system-ci
temporarily deployed
to
govuk-frontend-review-pr-763
We wrap the media query (mq) library with `govuk-media-query` so we can change it in the future. I think these may have been missed out when we originally did that work: #763 There is no impact on users that I'm aware of, so this is just a tidy up.
‘Abstract away’ sass-mq so that we don’t expose it as part of our public API, which means we’d be able to remove it in the future if we wanted to roll our own or move to a different vendored solution.
I've added tests for the
govuk-media-querymixin based off the existing tests for sass-mq, but I've removed the tests for the sass-mq functions that we're now not calling directly, as I don't think it makes sense to test the 'internals' of a vendored library when we can test its edges instead.