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

Review and merge switch from sass-lint to stylelint #1896

Closed
vanitabarrett opened this issue Aug 4, 2020 · 3 comments · Fixed by #1875
Closed

Review and merge switch from sass-lint to stylelint #1896

vanitabarrett opened this issue Aug 4, 2020 · 3 comments · Fixed by #1875

Comments

@vanitabarrett
Copy link
Contributor

vanitabarrett commented Aug 4, 2020

Part of #1740

What

Review the PR raised by GOVUK to switch to stylelint. Things to look out for:

  • check that the linting is working as expected as part of the build tasks
  • there are no unexpected code changes as as result of new linter config
  • we agree with all the linting rules that have been implemented
  • PR merged

Why

GOVUK needs to move away from scss-lint as it's blocking their upgrade from Ruby SASS to Sassc. They have chosen stylelint and have raised a PR against GOVUK Frontend to demonstrate how we may switch to stylelint to match. This will also be shared with the frontend community so we can aim for consistency across GDS.

We need to review this PR to make sure there are no unexpected changes.

@36degrees 36degrees mentioned this issue Aug 4, 2020
5 tasks
@vanitabarrett vanitabarrett linked a pull request Aug 4, 2020 that will close this issue
@vanitabarrett vanitabarrett added this to Upcoming sprints in Design System Sprint Board via automation Aug 4, 2020
@vanitabarrett vanitabarrett moved this from Upcoming sprints to Sprint Backlog in Design System Sprint Board Aug 4, 2020
@vanitabarrett vanitabarrett self-assigned this Aug 21, 2020
@vanitabarrett vanitabarrett moved this from Sprint Backlog to In progress in Design System Sprint Board Aug 21, 2020
@vanitabarrett vanitabarrett moved this from In progress to Needs review in Design System Sprint Board Aug 26, 2020
@vanitabarrett vanitabarrett moved this from Needs review to In progress in Design System Sprint Board Aug 27, 2020
@vanitabarrett
Copy link
Contributor Author

vanitabarrett commented Aug 28, 2020

Prefixes within govuk-device-pixel-ratio()

Kevin's PR currently removes min--moz-device-pixel-ratio and min-device-pixel-ratio (see PR), which we were a bit wary of in case they're needed.

Findings:

  • We need min--moz-device-pixel-ratio to support anything before Firefox 16
  • We need equivalent dpi and dppx to support some browsers, e.g: IE11
  • We can remove min-device-pixel-ratio as it never became an actual standard

(caniuse.com)

Suggestion: if we don't support Firefox 16, we can leave Kevin's commit as-is. Otherwise: remove min-device-pixel-ratio from the govuk-device-pixel-ratio mixin, but leave everything else as is. We disable the media-feature-name-no-unknown stylelint rule for min--moz-device-pixel-ratio.


Sidenote: I’m not 100% sure we’re using autoprefixer as intended. The prefixes are generated correctly for /dist when I condense the mixin down to just min-resolution, but NOT for /package.

Examples:

src file:

# Condensed mixin to just min-resolution, with $ratio
@mixin govuk-device-pixel-ratio($ratio: 2) {
  @media only screen and (min-resolution: #{($ratio*96)}dpi),
    only screen and (min-resolution: #{$ratio}dppx) {
      @content;
    }
}

Generated package ($ratio):

@mixin govuk-device-pixel-ratio($ratio: 2) {
  @media only screen and (min-resolution: #{($ratio*96)}dpi),
    only screen and (min-resolution: #{$ratio}dppx) {
      @content;
    }
}

src file (hardcoded numbers):

# Condensed mixin to just min-resolution, with fixed number instead of $ratio
@mixin govuk-device-pixel-ratio() {
  @media only screen and (min-resolution: 2dpi),
    only screen and (min-resolution: 2dppx) {
      @content;
    }
}

Generated package (fixed numbers):

@mixin govuk-device-pixel-ratio() {
  @media only screen and (-webkit-min-device-pixel-ratio: 0.020833333333333332),
    only screen and (min-resolution: 2dpi),
    only screen and (-webkit-min-device-pixel-ratio: 2),
    only screen and (min-resolution: 2dppx) {
      @content;
    }
}

It looks like it’s tripped up by the use of the $ratio variable. When I raised this as an issue, it's suggested we're not using autoprefixer as intended (on CSS rather than on SCSS): postcss/autoprefixer#1355. Not sure where else we’re relying on autoprefixer, but we may want to check/revisit our use of it

@vanitabarrett vanitabarrett moved this from In progress to Needs review in Design System Sprint Board Sep 1, 2020
@36degrees 36degrees added this to the v3.9.0 milestone Sep 1, 2020
@hannalaakso
Copy link
Member

I would be in favour of going with Kevin's commit and not making any other changes. FF 16 is quite far out of our support matrix and also the risk seems low in terms of what the impacted mixin does.

The autoprefixer issue seems worth investigating if you can raise an issue for us about it @vanitabarrett 🕵️‍♀️

@vanitabarrett
Copy link
Contributor Author

Autoprefixer issue created here: #1940

@vanitabarrett vanitabarrett changed the title Review switch from sass-lint to stylelint Review and merge switch from sass-lint to stylelint Sep 2, 2020
@36degrees 36degrees removed this from the v3.9.0 milestone Sep 9, 2020
@vanitabarrett vanitabarrett moved this from Needs review to Done in Design System Sprint Board Sep 14, 2020
@hannalaakso hannalaakso moved this from Done to Ready to release in Design System Sprint Board Sep 15, 2020
@vanitabarrett vanitabarrett moved this from Ready to release to Done in Design System Sprint Board Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

3 participants