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

348 migrate to dart sass #2278

Merged
merged 1 commit into from
Aug 28, 2024
Merged

348 migrate to dart sass #2278

merged 1 commit into from
Aug 28, 2024

Conversation

davidtrussler
Copy link
Contributor

Trello

This work makes the necessary changes to migrate Publisher to use Dart Sass. It follows the instructions in Migrate to Dart Sass from LibSass.

It is close to the changes made in PR2250 (Switch from sassc-rails to dartsass-rails) with a couple of slight changes that seem to fix the issues encountered there, specifically bringing the legacy styles into the SASS Builds folder via the initialiser that is created as part of the work.

Copy link
Contributor

@mtaylorgds mtaylorgds left a comment

Choose a reason for hiding this comment

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

Looks good, we just need to make a permission change to the bin/dev file and also merge in a change to the docker-compose.yml file in the govuk-docker repo.

bin/dev Show resolved Hide resolved
Copy link
Contributor

@mtaylorgds mtaylorgds left a comment

Choose a reason for hiding this comment

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

Kevin has commented on the govuk-docker PR on a better way of running the app, but that will involve some changes to this PR as well. I'm going to see if I can get that working locally.

Copy link
Contributor

@mtaylorgds mtaylorgds left a comment

Choose a reason for hiding this comment

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

We're also going to need to make some similar fixes as were made to Content Tagger to resolve issues with some icons not appearing/appearing corrupted.

image

@davidtrussler
Copy link
Contributor Author

Kevin has commented on the govuk-docker PR on a better way of running the app, but that will involve some changes to this PR as well. I'm going to see if I can get that working locally.

I have updated Procfile.dev in line with Kevin's suggestions in the Docker PR770. That seems to run fine locally.

@davidtrussler
Copy link
Contributor Author

We're also going to need to make some similar fixes as were made to Content Tagger to resolve issues with some icons not appearing/appearing corrupted.

image

Ah right, good spot. This seems to be fixed by referencing the Glyphicons font in the legacy-application sass file.

@davidtrussler
Copy link
Contributor Author

We're also going to need to make some similar fixes as were made to Content Tagger to resolve issues with some icons not appearing/appearing corrupted.

image

Hello @mtaylorgds - this has been addressed but needs to be marked as such before I am able to merge. Could you have a quick look? Cheers 🙂

- Initial installation
- Configuration
  - Add builds folder
  - Update the manifest file
  - Add an initializer
  - Delete unused Sass configuration
- Configure build options
- Replace asset helpers
- Update permissions for `bin/dev`
- Update `Procfile.dev` to include the worker process
- Add Glyphicons to legacy CSS file
@davidtrussler davidtrussler merged commit 0acdf2e into main Aug 28, 2024
13 checks passed
@davidtrussler davidtrussler deleted the 348_Migrate-to-DartSass branch August 28, 2024 13:49
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.

3 participants