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 #6634

Merged
merged 5 commits into from Jan 16, 2024
Merged

Migrate to Dart Sass from LibSass #6634

merged 5 commits into from Jan 16, 2024

Conversation

matthillco
Copy link
Contributor

@matthillco matthillco commented Dec 20, 2023

What

Migrate Smart Answers to Dart Sass from LibSass Trello

Why

Visual Changes

None.

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:

@govuk-ci govuk-ci temporarily deployed to smart-answers-pr-6634 December 20, 2023 16:10 Inactive
@govuk-ci govuk-ci temporarily deployed to smart-answers-pr-6634 December 20, 2023 16:43 Inactive
@govuk-ci govuk-ci temporarily deployed to smart-answers-pr-6634 December 20, 2023 17:08 Inactive
@govuk-ci govuk-ci temporarily deployed to smart-answers-pr-6634 December 21, 2023 16:54 Inactive
@govuk-ci govuk-ci temporarily deployed to smart-answers-pr-6634 December 22, 2023 14:16 Inactive
@govuk-ci govuk-ci temporarily deployed to smart-answers-pr-6634 December 22, 2023 14:25 Inactive
@govuk-ci govuk-ci temporarily deployed to smart-answers-pr-6634 December 22, 2023 15:02 Inactive
Copy link
Contributor

@jon-kirwan jon-kirwan left a comment

Choose a reason for hiding this comment

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

Thanks Matt, great work. All looks good and works fine. I've just added a couple of small comments and a general note around vendor CSS

bin/dev Show resolved Hide resolved
config/initializers/dartsass.rb Show resolved Hide resolved
config/environments/production.rb Outdated Show resolved Hide resolved
config/initializers/dartsass.rb Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@jon-kirwan
Copy link
Contributor

jon-kirwan commented Jan 4, 2024

I'm adding a note regarding vendor CSS which is currently uncompressed. It seems that joint.css is designed for internal use, meant to visualize Smart Answer journeys. To compress CSS files, it looks like the typical approach involves adding Sass configuration, like this:

config.sass.style = :compressed
config.sass.line_comments = false

However, this process seems closely linked to the sassc-rails gem, which has been removed. The only alternative I've come across for CSS compression is to add config.assets.css_compressor = :yui which requires the yui-compressor gem https://guides.rubyonrails.org/asset_pipeline.html#css-compression.

At present, the vendor CSS folder doesn't seem to be used in other UX applications that are being migrated to Dart Sass.

Otherwise, Sass files are compressed before being handled by the asset pipeline.

@govuk-ci govuk-ci temporarily deployed to smart-answers-pr-6634 January 9, 2024 15:02 Inactive
@govuk-ci govuk-ci temporarily deployed to smart-answers-pr-6634 January 9, 2024 15:18 Inactive
@matthillco
Copy link
Contributor Author

@jon-kirwan I've made the requested changes, please could you have a final check? Thanks.

@matthillco matthillco marked this pull request as ready for review January 9, 2024 15:23
@matthillco matthillco self-assigned this Jan 9, 2024
@matthillco matthillco changed the title Migrate to Dart Sass from LibSass [DO NOT MERGE] Migrate to Dart Sass from LibSass Jan 9, 2024
Copy link
Contributor

@jon-kirwan jon-kirwan left a comment

Choose a reason for hiding this comment

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

All working fine for me and the changes look great! Thanks Matt 👍

- 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
@govuk-ci govuk-ci temporarily deployed to smart-answers-pr-6634 January 16, 2024 12:17 Inactive
@matthillco matthillco merged commit 5cb2df7 into main Jan 16, 2024
13 checks passed
@matthillco matthillco deleted the migrate-to-dart-sass branch January 16, 2024 15:33
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.

None yet

3 participants