MPP-3946: Fix warnings for mixed_decls, imports #5176

jwhitlock · 2024-11-05T22:07:11Z

This PR replaces PR #5174.

This PR addresses the Sass 1.80 deprecation warnings about mixed declarations and @import statements. Whenever possible, I used sass-migrator to update the files.

My goals are:

Generate the exact same CSS, so that CSS knowledge would not be needed to review (or write) this PR. Future PRs will need to change the CSS.
Minimize code changes, even though every .sass file needed updates.

Some of the changes:

@import declarations are changed to @use. This was done by sass-migrator, which knows the details of variable resolution.
Standardize imports:
- @use "~@mozilla-protocol/core/protocol/css/includes/lib" as *; brings the most common mozilla-protocol variables into the global namespace. Replace some alternatives that used the node_modules path.
- @use "~@mozilla-protocol/core/protocol/css/includes/forms/lib" as mp_forms; for the frequently used forms variables.
- styles/tokens became styles/color, since just about everything was a color.
In styles/color (was styles/tokens.scss), also remove the color- prefix. This means that the Nebula colors look like .color$light-gray-90, and the Protocol colors look like $color-light-gray-90. I'm not sure if one or the other was intended, but now it is a lot clearer which was used where. Did I do it correctly? sass-migrator made the changes, and the CSS is identical.
Add styles/text, re-implement the Protocol text @mixins to take a @content block.
Added style/README.md to document these files.

Here's an example of the @mixin update. This code:

@import "~@mozilla-protocol/core/protocol/css/includes/lib";
 
h2 {
  @include text-title-xs;
  font-family: $font-stack-firefox;
  text-align: center;
}

becomes:

@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
@use "../../../styles/text";

h2 {
  @include text.title-xs {
    font-family: $font-stack-firefox;
    text-align: center;
  }
}

How to test

Run cd frontend; npm run build.

The .map files in the same folder, such as /frontend/out/_next/static/css/135df0ac639034c3.css.map, have changed because the source file changed.

groovecoder

Code looks good - no blockers. Still need to run the compile and check things out.

.github/pull_request_template.md

groovecoder · 2024-11-11T17:20:19Z

frontend/next.config.js

    // TODO MPP-3946: Fix deprecation warnings in sass 1.80.x
    // https://github.com/mozilla/protocol/releases/tag/v18.0.0


questgestion (non-blocking): we can remove these lines, right? This TODO is TODONE?

Suggested change

// TODO MPP-3946: Fix deprecation warnings in sass 1.80.x

// https://github.com/mozilla/protocol/releases/tag/v18.0.0

No, there are sass warnings from mozilla-protocol, and the lingering next.js API warning.

groovecoder · 2024-11-11T17:28:50Z

frontend/src/components/dashboard/EmailForwardingModal.module.scss

-  padding: $spacing-sm 0;
-  gap: $spacing-sm;
-  font-weight: 100;
+  @include text.title-xs {


question (non-blocking): TIL sass mixin content blocks. Did you dig into them any deeper than to update the syntax? Do you know if we should file a follow-up to try to utilize the new features of mixin content blocks to improve our sass code in other ways too?

Yes, I think our style definitions should be updated with this in mind. No, I don't think it is worth a ticket, since we would close it as 'not planned' if someone else suggested it. I've thought a bit about it, so I'm writing my blog post here, but feel free to skip it.

We have no experienced front-end developers on staff, and I bet once we hire one, 'clean up style sheets' will be well below other tasks the design team is planning. In the end, we will probably follow the Mozilla Protocol CSS Coding Guide, after they figure out what to do with this change.

The goal with this update was to generate the exact same CSS, with no judgement or re-arrangement. This was mostly because I'm not qualified to judge if the CSS has the same effect, or if there is some gotcha in Chrome vs Firefox vs Vivaldi etc. It was also easiest to judge the output - same hash, same CSS.

This declaration keeps the CSS the same:

.headline { @include text.title-xs { display: flex; align-items: center; justify-content: center; padding: $spacing-sm 0; gap: $spacing-sm; font-weight: 100; } }

The CSS order is 1) the text.title-xs properties, then 2) the content block properties, then 3) the desktop-width text.title-xs overrides:

.EmailForwardingModal_headline__VgCSC { font-size:24px; font-size:1.5rem; line-height:1.08; display:flex; align-items:center; justify-content:center; padding:8px 0; gap:8px; font-weight:100 } @media(min-width:768px) { .EmailForwardingModal_headline__VgCSC{ font-size:28px; font-size:1.75rem; line-height:1.07 } }

The post Poll Results: How do you order your CSS properties? says the strategies are:

45% - Grouped by Type

39% - Randomly

14% - Alphabetical

2% - By line length

Our coding standards doc says:

We follow Protocol's CSS coding guide. See their documentation for additional details.

That document says:

Order declarations alphabetically (from A to Z).

I think the Relay team has not followed that one, because our linters do not enforce it.

If we grouped by type, such as Nicolas Gallagher's idiomatic CSS, which makes the most sense to me (an amateur front-end dev), the sass would be (without the comments):

.headline { /* No positioning properties */ /* Display and Box Model properties*/ display: flex; padding: $spacing-sm 0; gap: $spacing-sm; align-items: center; justify-content: center; /* No color properties */ /* Text properties */ @include text.title-xs { font-weight: 100; } }

This requires knowing what attributes go in what section. I had to use MDN to guess. This would be a good job for a linter.

A hypothetical alphabetical version would need to split the text.title-xs font declarations from the desktop-size overrides:

.headline { align-items: center; display: flex; @include text.font-title-xs { font-weight: 100; } gap: $spacing-sm; justify-content: center; padding: $spacing-sm 0; @include text.media-desktop-title-xs; }

This is ugly, requiring developers to remember to include both at-includes, so seems like a bad solution to me.

The marketing organization has a concentration of front-end developers. They have the same mixed-decls warning. They can address it by changing mozilla-protocol. They've opened mozilla/protocol#998 to discuss it. I expect that it will be solved in a future release of mozilla-protocol, and battle-tested in bedrock.

When they have a solution, we can update our code to match it. Our job is to make such that is a one-version update, not several versions like the present case. And maybe have a front-end dev on staff who can contribute to the conversation.

groovecoder · 2024-11-11T17:31:14Z

frontend/src/components/dashboard/aliases/BlockLevelSlider.module.scss

+  @media screen and #{$mq-md} {
+    --thumbDiameter: 24px;
+  }
+


question (non-blocking): Do you know if this block moved for some specific reason? Did the migrator move it?

I manually moved it to get rid of the mixed declaration warning. This code is a better example of the warning than our @include text-title-xs code, which requires reading code across two repositories. Currently, Dart Sass manually moves the nested @media screen and #{$mq-md} { code below the top-level properties display: flex through width: 100%, but won't in the future.

frontend/src/components/dashboard/tips/GenericTip.module.scss

groovecoder · 2024-11-11T17:36:14Z

frontend/src/components/landing/BundleBanner.module.scss

@@ -45,7 +46,7 @@

    .main-img-wrapper {
      .main-image {
-        margin-top: -$layout-lg; // Leave space for image to bleed out of frame
+        margin-top: -($layout-lg); // Leave space for image to bleed out of frame


praise: nice catch. did you make this, or the migrator?

I don't know when that changed...

looks at git blame

Ok, the module migrator first changed it to:

@use "~@mozilla-protocol/core/protocol/css/includes/lib" as lib; .main-img-wrapper { margin-top: -(lib.$layout-lg); // Leave space for image to bleed out of frame }

Then I used the --rename option to change from lib. to .mp for mozilla-protocol:

@use "~@mozilla-protocol/core/protocol/css/includes/lib" as mp; .main-img-wrapper { margin-top: -(mp.$layout-lg); // Leave space for image to bleed out of frame }

Then I used the --rename option to change from mp. to * for mozilla-protocol, which didn't quite work:

@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *; .main-img-wrapper { margin-top: -(*.$layout-lg); // Leave space for image to bleed out of frame }

so I had to manually (sed) change it to not treat * as a namespace:

@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *; .main-img-wrapper { margin-top: -($layout-lg); // Leave space for image to bleed out of frame }

So, generates the same CSS, but maybe should be reverted to the original...

groovecoder · 2024-11-11T17:49:52Z

frontend/src/styles/README.md

+@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
+@use "../styles/color";


question (non-blocking): These Mozilla Protocol @use statements are identical to the Nebula ones above? Are they supposed to be different? Or is it really the case that the Nebula color variables are in the color namespace and the Protocol colors are $color-* variables in the global namespace? I think you mention this in the PR description, but maybe add that explanation to this README.md too?

yeah, that is not clear to me either. The CSS uses the the actual values (like color:#ffa436), and the CSS didn't change :broken_record:.

I think it is a separate task to confirm we used the colors we intended to use, since that may change the CSS, and may break E2E screenshot tests. I've opened MPP-3958 for that work, I'll add a reference to the README.

groovecoder

The first time I ran npm run build, I got warnings but they were for 3rd-party modules - not our code. All runs after the first run finished with no warnings.

jwhitlock

A few changes to make:

Revert change to frontend/src/components/landing/BundleBanner.module.scss
Add MPP-3958 to README.md
Add MPP-3959 to TODO

jwhitlock · 2024-11-12T16:30:31Z

frontend/src/components/dashboard/EmailForwardingModal.module.scss

-  padding: $spacing-sm 0;
-  gap: $spacing-sm;
-  font-weight: 100;
+  @include text.title-xs {


Yes, I think our style definitions should be updated with this in mind. No, I don't think it is worth a ticket, since we would close it as 'not planned' if someone else suggested it. I've thought a bit about it, so I'm writing my blog post here, but feel free to skip it.

We have no experienced front-end developers on staff, and I bet once we hire one, 'clean up style sheets' will be well below other tasks the design team is planning. In the end, we will probably follow the Mozilla Protocol CSS Coding Guide, after they figure out what to do with this change.

The goal with this update was to generate the exact same CSS, with no judgement or re-arrangement. This was mostly because I'm not qualified to judge if the CSS has the same effect, or if there is some gotcha in Chrome vs Firefox vs Vivaldi etc. It was also easiest to judge the output - same hash, same CSS.

This declaration keeps the CSS the same:

.headline { @include text.title-xs { display: flex; align-items: center; justify-content: center; padding: $spacing-sm 0; gap: $spacing-sm; font-weight: 100; } }

The CSS order is 1) the text.title-xs properties, then 2) the content block properties, then 3) the desktop-width text.title-xs overrides:

.EmailForwardingModal_headline__VgCSC { font-size:24px; font-size:1.5rem; line-height:1.08; display:flex; align-items:center; justify-content:center; padding:8px 0; gap:8px; font-weight:100 } @media(min-width:768px) { .EmailForwardingModal_headline__VgCSC{ font-size:28px; font-size:1.75rem; line-height:1.07 } }

The post Poll Results: How do you order your CSS properties? says the strategies are:

45% - Grouped by Type

39% - Randomly

14% - Alphabetical

2% - By line length

Our coding standards doc says:

We follow Protocol's CSS coding guide. See their documentation for additional details.

That document says:

Order declarations alphabetically (from A to Z).

I think the Relay team has not followed that one, because our linters do not enforce it.

If we grouped by type, such as Nicolas Gallagher's idiomatic CSS, which makes the most sense to me (an amateur front-end dev), the sass would be (without the comments):

.headline { /* No positioning properties */ /* Display and Box Model properties*/ display: flex; padding: $spacing-sm 0; gap: $spacing-sm; align-items: center; justify-content: center; /* No color properties */ /* Text properties */ @include text.title-xs { font-weight: 100; } }

This requires knowing what attributes go in what section. I had to use MDN to guess. This would be a good job for a linter.

A hypothetical alphabetical version would need to split the text.title-xs font declarations from the desktop-size overrides:

.headline { align-items: center; display: flex; @include text.font-title-xs { font-weight: 100; } gap: $spacing-sm; justify-content: center; padding: $spacing-sm 0; @include text.media-desktop-title-xs; }

This is ugly, requiring developers to remember to include both at-includes, so seems like a bad solution to me.

The marketing organization has a concentration of front-end developers. They have the same mixed-decls warning. They can address it by changing mozilla-protocol. They've opened mozilla/protocol#998 to discuss it. I expect that it will be solved in a future release of mozilla-protocol, and battle-tested in bedrock.

When they have a solution, we can update our code to match it. Our job is to make such that is a one-version update, not several versions like the present case. And maybe have a front-end dev on staff who can contribute to the conversation.

jwhitlock · 2024-11-12T16:36:31Z

frontend/src/components/dashboard/aliases/BlockLevelSlider.module.scss

+  @media screen and #{$mq-md} {
+    --thumbDiameter: 24px;
+  }
+


I manually moved it to get rid of the mixed declaration warning. This code is a better example of the warning than our @include text-title-xs code, which requires reading code across two repositories. Currently, Dart Sass manually moves the nested @media screen and #{$mq-md} { code below the top-level properties display: flex through width: 100%, but won't in the future.

jwhitlock · 2024-11-12T16:49:58Z

frontend/src/components/landing/BundleBanner.module.scss

@@ -45,7 +46,7 @@

    .main-img-wrapper {
      .main-image {
-        margin-top: -$layout-lg; // Leave space for image to bleed out of frame
+        margin-top: -($layout-lg); // Leave space for image to bleed out of frame


I don't know when that changed...

looks at git blame

Ok, the module migrator first changed it to:

@use "~@mozilla-protocol/core/protocol/css/includes/lib" as lib; .main-img-wrapper { margin-top: -(lib.$layout-lg); // Leave space for image to bleed out of frame }

Then I used the --rename option to change from lib. to .mp for mozilla-protocol:

@use "~@mozilla-protocol/core/protocol/css/includes/lib" as mp; .main-img-wrapper { margin-top: -(mp.$layout-lg); // Leave space for image to bleed out of frame }

Then I used the --rename option to change from mp. to * for mozilla-protocol, which didn't quite work:

@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *; .main-img-wrapper { margin-top: -(*.$layout-lg); // Leave space for image to bleed out of frame }

so I had to manually (sed) change it to not treat * as a namespace:

@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *; .main-img-wrapper { margin-top: -($layout-lg); // Leave space for image to bleed out of frame }

So, generates the same CSS, but maybe should be reverted to the original...

jwhitlock · 2024-11-12T17:11:17Z

frontend/src/styles/README.md

+@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
+@use "../styles/color";


yeah, that is not clear to me either. The CSS uses the the actual values (like color:#ffa436), and the CSS didn't change :broken_record:.

I think it is a separate task to confirm we used the colors we intended to use, since that may change the CSS, and may break E2E screenshot tests. I've opened MPP-3958 for that work, I'll add a reference to the README.

jwhitlock · 2024-11-12T17:15:30Z

frontend/src/styles/globals.scss

@@ -22,6 +22,7 @@ a {
 /*
  Josh's Custom CSS Reset
  https://www.joshwcomeau.com/css/custom-css-reset/
+  TODO: Try changes from June 2023 and later


I opened MPP-3959 for this TODO

@content

Copy some of the text-* mixins to a new file /frontend/src/styles/typography.scss Modify the mixins to accept a @content parameter. Use these new mixins to generate the same CSS without the SCSS deprecation warning for mixed declarations: https://sass-lang.com/documentation/breaking-changes/mixed-decls/

This is mostly done with: find . -type f -name '*.scss' | xargs npx sass-migrator module with some custom work to get the build working, and an extra comment to make the generated CSS exactly the same.

Use the namespace 'mp' for main mozilla/protocol lib, and namespace 'mp_forms' for forms library. Converted with: find . -type f -name '*.scss' | xargs npx sass-migrator namespace -r \ "url ~@mozilla-protocol/core/protocol/css/includes/lib to mp; url ~@mozilla-protocol/core/protocol/css/includes/forms/lib to mp_forms"

Order the at-use statements in a consistent way.

jwhitlock · 2024-11-12T17:46:41Z

Rebased on main to get cookie change in PR #5175

jwhitlock marked this pull request as draft November 5, 2024 22:07

jwhitlock force-pushed the sass-1-80-imports-mpp-3946 branch 3 times, most recently from fb05943 to dda727d Compare November 6, 2024 20:15

jwhitlock marked this pull request as ready for review November 6, 2024 20:42

jwhitlock mentioned this pull request Nov 6, 2024

MPP-3946: Add @content to mozilla protocol text mixins #5174

Closed

11 tasks

jwhitlock requested a review from groovecoder November 6, 2024 21:04

jwhitlock assigned groovecoder and jwhitlock Nov 7, 2024

groovecoder reviewed Nov 11, 2024

View reviewed changes

groovecoder approved these changes Nov 11, 2024

View reviewed changes

jwhitlock commented Nov 12, 2024

View reviewed changes

jwhitlock enabled auto-merge November 12, 2024 17:44

jwhitlock added 13 commits November 12, 2024 11:45

Convert SCSS from at-import to at-use

4b0a43a

This is mostly done with: find . -type f -name '*.scss' | xargs npx sass-migrator module with some custom work to get the build working, and an extra comment to make the generated CSS exactly the same.

Use mozilla protocol w/o namespace

78f2461

Use t for styles/typography

711fcc2

Standardize at-use urls

920bf6a

Rename to styles/text

2b00198

Rename to styles/color

4ce9832

Order at-use statements

86647a3

Order the at-use statements in a consistent way.

Add README, move long comment, update refs

8c1bbca

Revert parens

7cd7a06

MPP-3958: Note that color audit is needed

c1f42b6

MPP-3959: Note that updates are available

cf181ba

jwhitlock force-pushed the sass-1-80-imports-mpp-3946 branch from f6eb419 to cf181ba Compare November 12, 2024 17:45

jwhitlock added this pull request to the merge queue Nov 12, 2024

Merged via the queue into main with commit 2e286b9 Nov 12, 2024
30 checks passed

jwhitlock deleted the sass-1-80-imports-mpp-3946 branch November 12, 2024 18:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MPP-3946: Fix warnings for mixed_decls, imports #5176

MPP-3946: Fix warnings for mixed_decls, imports #5176

jwhitlock commented Nov 5, 2024 •

edited

Loading

groovecoder left a comment

groovecoder Nov 11, 2024

jwhitlock Nov 12, 2024

groovecoder Nov 11, 2024

jwhitlock Nov 12, 2024

groovecoder Nov 11, 2024

jwhitlock Nov 12, 2024

groovecoder Nov 11, 2024

jwhitlock Nov 12, 2024

groovecoder Nov 11, 2024

jwhitlock Nov 12, 2024

groovecoder left a comment

jwhitlock left a comment

jwhitlock Nov 12, 2024

jwhitlock Nov 12, 2024

jwhitlock Nov 12, 2024

jwhitlock Nov 12, 2024

jwhitlock Nov 12, 2024

jwhitlock commented Nov 12, 2024

		// TODO MPP-3946: Fix deprecation warnings in sass 1.80.x
		// https://github.com/mozilla/protocol/releases/tag/v18.0.0

		@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
		@use "../styles/color";

MPP-3946: Fix warnings for mixed_decls, imports #5176

MPP-3946: Fix warnings for mixed_decls, imports #5176

Conversation

jwhitlock commented Nov 5, 2024 • edited Loading

How to test

groovecoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

groovecoder left a comment

Choose a reason for hiding this comment

jwhitlock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwhitlock commented Nov 12, 2024

jwhitlock commented Nov 5, 2024 •

edited

Loading