-
Notifications
You must be signed in to change notification settings - Fork 328
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
Deprecate govuk/all.scss
and only reference govuk/index.scss
internally
#5518
base: main
Are you sure you want to change the base?
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 1720493 |
Other changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/all.scss b/packages/govuk-frontend/dist/govuk/all.scss
index 1d57cf86e..b29cb4977 100644
--- a/packages/govuk-frontend/dist/govuk/all.scss
+++ b/packages/govuk-frontend/dist/govuk/all.scss
@@ -1,3 +1,9 @@
@import "index";
+@include _warning(
+ "import-using-all",
+ "Importing using 'govuk/all' is deprecated. Update your import statement to import 'govuk/index'.",
+ $silence-further-warnings: false
+);
+
/*# sourceMappingURL=all.scss.map */
Action run for 1720493 |
bd08d47
to
276c806
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blimey, that was a lot of places we were using that all.scss
file, thanks for being super thorough, as always 🙌🏻 .
Only two minor comments on the update, thanks. I think it also needs a CHANGELOG entry announcing the deprecation 😊
276c806
to
069d35e
Compare
govuk/all.scss
and only reference govuk/index.scss
internally
069d35e
to
831df3c
Compare
831df3c
to
9fca741
Compare
I've added a changelog that mostly replicates what was written for when we deprecated the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers for the CHANGELOG and taking onboard the suggestions 🙌🏻 I've pulled @calvin-lau-sig7 and @seaemsi to ask when they prefer to review the content, and noticed two little things in the code that raised questions (and that I may have missed in my initial review).
#### Importing Sass using `govuk/all` | ||
|
||
You'll see a warning when compiling your Sass if you import all of GOV.UK Frontend's styling via `govuk/all`. Importing using the `all` file is deprecated, and we’ll remove it in the next major release. | ||
|
||
In your import statements, use a trailing `/index` rather than `/all` to load GOV.UK Frontend's files: | ||
|
||
- `@import "govuk/index";` instead of `@import "govuk/all";`; | ||
|
||
You do not need `/index` at the end of each import path if you’re using Dart Sass, LibSass 3.6.0 or higher, or Ruby Sass 3.6.0 or higher. | ||
|
||
This change was introduced in [pull request #5518: Deprecate `govuk/all.scss` and only reference `govuk/index.scss` internally](https://github.com/alphagov/govuk-frontend/pull/5518). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@calvin-lau-sig7 @seaemsi Would you prefer updating this bit as part of this PR or during the release process? Let us know what work best with your current workload 😊
|
||
- `@import "govuk/index";` instead of `@import "govuk/all";`; | ||
|
||
You do not need `/index` at the end of each import path if you’re using Dart Sass, LibSass 3.6.0 or higher, or Ruby Sass 3.6.0 or higher. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion Could we use this as an opportunity to remind that people should move to use Dart Sass rather than stay on LibSass or Ruby Sass or would that be too distracting?
You do not need `/index` at the end of each import path if you’re using Dart Sass, LibSass 3.6.0 or higher, or Ruby Sass 3.6.0 or higher. | |
You do not need `/index` at the end of each import path if you’re using Dart Sass (as well as LibSass 3.6.0 or higher, or Ruby Sass 3.6.0 or higher, though we recommend you migrate to Dart Sass if you're using LibSass or Ruby Sass as they are both deprecated). |
Or maybe by just silencing that you have benefits in the deprecated Sass compilers?
You do not need `/index` at the end of each import path if you’re using Dart Sass, LibSass 3.6.0 or higher, or Ruby Sass 3.6.0 or higher. | |
You do not need `/index` at the end of each import path if you’re using Dart Sass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer your second suggestion. Suggestion via erasure. It delivers the message (that messag being 'use Dart Sass') more subtly but also more efficiently. I'll wait and see what the content folks say.
9fca741
to
1720493
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good code-wise, thanks for making the changes. Let's see how we prefer handling the changelog 😊
A continuation of #4955 to deprecate the root
all
sass file as well as theall
files in the layers. Additionally removes all references toall.scss
internally.Part of #4988 but doesn't resolve it as we also need to update the Frontend docs.