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

Migrate to Dart Sass from LibSass #1655

Merged
merged 4 commits into from
Dec 19, 2023
Merged

Migrate to Dart Sass from LibSass #1655

merged 4 commits into from
Dec 19, 2023

Conversation

jon-kirwan
Copy link
Contributor

@jon-kirwan jon-kirwan commented Nov 24, 2023

What

Migrate to Dart Sass from LibSass.

Why

Visual Changes

None.

How can I test these changes?

  • run yarn jasmine:prepare (or run bundle exec rake app:assets:clobber app:assets:precompile)
  • what should happen? The Sass files should be compiled. Keep an eye on the command line for warnings indicating the compilation process using Dart Sass
Deprecation Warning: Using / for division outside of calc() is deprecated and will be removed in Dart Sass 2.0.0.
Recommendation: math.div(300%, 4) or calc(300% / 4)
More info and automated migrator: https://sass-lang.com/d/slash-div
   │
26 │   three-quarters: (300% / 4),
   │                    ^^^^^^^^
   ...
  • the /builds folder: app/assets/builds should now be populated with CSS files. Note, the
  • subsequently, all assets ought to be generated successfully within the /public directory
  • no test failures to indicate the correct number of stylesheets are generated
  • deploy to Integration. You'll notice that in the 'build and publish image' task, the steps taken resemble those mentioned earlier, dartsass-rails is bundled and any Dart Sass warnings encountered during the compilation process are reported)
  • inspect several pages, e.g.
    manage your GOV.UK email subscriptions
    , what do you want to get emails about?, get emails from GOV.UK, how often do you want to get emails? to ensure the absence of errors.

Anything else

Once the PR gets the green light, any temporary commits and references to local gems e.g. gem "govuk_publishing_components", path: "../govuk_publishing_components" will be removed.

See:

@jon-kirwan jon-kirwan changed the title Migrate to dart sass Migrate to Dart Sass from LibSass Nov 24, 2023
@jon-kirwan jon-kirwan force-pushed the migrate-to-dart-sass branch 2 times, most recently from 05d0417 to de4e38a Compare November 24, 2023 12:03
@jon-kirwan jon-kirwan marked this pull request as ready for review November 24, 2023 15:57
Copy link
Contributor

@MartinJJones MartinJJones left a comment

Choose a reason for hiding this comment

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

Works great for me when testing locally 👍

Just wanted to ask if we will also include the generated files from running the ./bin/rails dartsass:install command to help watch for any CSS changes, similar to alphagov/government-frontend@1ed1fae

If so, I think we would also need to create a separate PR to add support for this in govuk-docker

@jon-kirwan jon-kirwan force-pushed the migrate-to-dart-sass branch 2 times, most recently from db259d5 to 3f9830d Compare November 29, 2023 14:41
@jon-kirwan
Copy link
Contributor Author

Works great for me when testing locally 👍

Just wanted to ask if we will also include the generated files from running the ./bin/rails dartsass:install command to help watch for any CSS changes, similar to alphagov/government-frontend@1ed1fae

I'm on the fence about adding it here because of the limited number of pages (perhaps around 4?) and the difficulties of testing frontend changes without being signed in but perhaps I should consider enabling it here? What do you think? It might have been useful with the recent cross service header changes.

@MartinJJones
Copy link
Contributor

MartinJJones commented Nov 29, 2023

I'm on the fence about adding it here because of the limited number of pages (perhaps around 4?) and the difficulties of testing frontend changes without being signed in but perhaps I should consider enabling it here? What do you think? It might have been useful with the recent cross service header changes.

Thanks, these are good points, sounds reasonable to me for this application, and you have already added a section on this in the developer docs should another developer want to add support for this in the future - https://github.com/alphagov/govuk-developer-docs/blob/3bac7fd893e0442f1e1afd3a28bdebe600bd5174/source/manual/migrate-to-dart-sass-from-libsass.html.md#watch-mode

Copy link
Contributor

@MartinJJones MartinJJones left a comment

Choose a reason for hiding this comment

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

Changes look good to me, nice work!

Happy to approve the PR once the merge conflict is resolved, I think this will likely solve itself when testing is complete and "TEMP" commits removed/squashed.

@jon-kirwan jon-kirwan force-pushed the migrate-to-dart-sass branch 2 times, most recently from 671c397 to 303c1dd Compare December 4, 2023 16:18
Copy link
Contributor

@MartinJJones MartinJJones left a comment

Choose a reason for hiding this comment

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

Changes look good to me 👍

- Create `dartsass.builds` initializer and add all Sass files to be compiled. See https://github.com/rails/dartsass-rails#configuring-builds
- Create .keep
- Update manifest file to use /builds directory
- Ignore /builds
@jon-kirwan jon-kirwan merged commit 04e1243 into main Dec 19, 2023
11 checks passed
@jon-kirwan jon-kirwan deleted the migrate-to-dart-sass branch December 19, 2023 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants