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

Format Sass stylesheets using Prettier #3799

Merged
merged 10 commits into from
Aug 7, 2023
Merged

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Jun 12, 2023

This PR updates stylelint-config-gds with Stylelint 15 and now includes:

  • Checks for media query syntax, rules and values
  • Checks for number precision inside CSS functions
  • Fixes for duplicate rules with intentionally different units

Breaking change: Removal of stylistic rules

Stylelint now recommends Prettier for formatting in their Migrating to 15.0.0 guide

I've run prettier --write on all stylesheets as recommended

We have removed all stylistic rules (such as tabs/spaces, indentation, etc) as they're no longer included in Stylelint common configurations as part of their deprecation in Stylelint 15.

As per Stylelint's own documentation, we recommend that projects adopt Prettier for formatting instead.

If this is not possible for your project, you can configure your projects' Stylelint configuration to use the stylelint-stylistic or stylelint-codeguide plugins to restore the deprecated rules instead.

This change was made in pull request #44: Remove deprecated stylistic rules.

package.json Outdated
"stylelint": "^14.16.1",
"stylelint-config-gds": "^0.3.0",
"stylelint": "^15.7.0",
"stylelint-config-gds": "github:alphagov/stylelint-config-gds#stylelint-15",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Installing from GitHub branch until this PR is released:

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Jun 12, 2023

Seems to look good, but I'll highlight some not-too-pretty examples:

https://github.com/alphagov/govuk-frontend/pull/3799/files#diff-e9de60885fa511018982360bd5f64b5c55a56363d5653e65a6432cc3a6beef33R76

@include govuk-responsive-margin(
  6,
  "bottom",
  $adjustment: $button-shadow-size
); // s2
padding: (govuk-spacing(2) - $govuk-border-width-form-element)
  govuk-spacing(2)
  (
    govuk-spacing(2) - $govuk-border-width-form-element -
      ($button-shadow-size / 2)
  ); // s1

https://github.com/alphagov/govuk-frontend/pull/3799/files#diff-09fe9243587018027ad713666b9b6c114728a2ea2a227ea9265ce482dbfbb9adR35

.govuk-fieldset__legend:not(.govuk-fieldset__legend--m):not(
    .govuk-fieldset__legend--l
  ):not(.govuk-fieldset__legend--xl)
  + .govuk-hint {
  margin-bottom: govuk-spacing(2);
}

Update: Prettier line length at 120 characters is better, plus some manual intervention in ccaf200 🤷‍♂️

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 June 14, 2023 17:07 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 June 14, 2023 19:20 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 June 15, 2023 07:24 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 June 15, 2023 18:07 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 June 20, 2023 08:28 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 June 20, 2023 08:47 Inactive
@colinrotherham colinrotherham marked this pull request as draft June 20, 2023 15:28
@romaricpascal
Copy link
Member

On my side, pretty happy with a move to Prettier for the formatting, especially as we can format pre-commit. Would be keen to use it for formatting JS as well, and possibly Nunjucks so we’re consistent all across. I think the gain from not having to think about formatting anymore outweighs the couple of places where a manual formatting may yield something a little less surprising or preserve some alignment.

A wider line width doesn't sound bad either, especially with selectors that can quickly get quite long so it sounds fine to bump it up.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 June 27, 2023 09:42 Inactive
@querkmachine
Copy link
Member

querkmachine commented Jun 27, 2023

+1 for having Prettier deal with JS and Sass formatting automagically.

Having tried using it for Nunjucks however, there seem to be few options available that work and fewer still that work well. There are some Prettier plugins for similar languages like Jinja and Twig that also work when applied to Nunjucks files, but they're often hardcoded with the stylistic preferences of the authors and trip over in places where the control structures are slightly different.

If we wanna make our own Prettier plugin for Nunjucks then I think plenty of people would appreciate that, though! 😋

Edit: This is the package I've used in the past for reference's sake.

@colinrotherham colinrotherham self-assigned this Jun 27, 2023
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 June 29, 2023 10:21 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 August 4, 2023 10:39 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 August 4, 2023 11:17 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 August 4, 2023 11:18 Inactive
@colinrotherham colinrotherham changed the base branch from main to linting-docs August 4, 2023 11:18
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 August 4, 2023 11:56 Inactive
@colinrotherham
Copy link
Contributor Author

Thanks @romaricpascal

As discussed on Slack I've added some Prettier documentation and also opened:

Copy link
Member

@querkmachine querkmachine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for having Prettier deal with JS and Sass formatting automagically.

How's this branch for you @querkmachine? Ready for review now

Hopefully all playing nice with Nova

@colinrotherham Oop, missed this comment before.

Having tried the most recent push, I can confirm that it's playing nicely with Nova! Happy with the formatting changes too.

Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the updates and documenting things in another PR (I'll have a look on Monday)

Base automatically changed from linting-docs to main August 7, 2023 17:01
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3799 August 7, 2023 17:07 Inactive
@colinrotherham colinrotherham merged commit 362c623 into main Aug 7, 2023
40 checks passed
@colinrotherham colinrotherham deleted the stylelint-prettier branch August 7, 2023 17:21
@colinrotherham colinrotherham removed their assignment Dec 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants