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.

Sign up for GitHub

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

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

Merged
merged 13 commits into from
Nov 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5ec573b
Add at-content to mozilla protocol text mixins
jwhitlock Nov 4, 2024
4b0a43a
Convert SCSS from at-import to at-use
jwhitlock Nov 5, 2024
0838adc
Use namespaces mp and mp_forms
jwhitlock Nov 5, 2024
78f2461
Use mozilla protocol w/o namespace
jwhitlock Nov 5, 2024
711fcc2
Use t for styles/typography
jwhitlock Nov 5, 2024
920bf6a
Standardize at-use urls
jwhitlock Nov 5, 2024
2b00198
Rename to styles/text
jwhitlock Nov 5, 2024
4ce9832
Rename to styles/color
jwhitlock Nov 5, 2024
86647a3
Order at-use statements
jwhitlock Nov 6, 2024
8c1bbca
Add README, move long comment, update refs
jwhitlock Nov 6, 2024
7cd7a06
Revert parens
jwhitlock Nov 12, 2024
c1f42b6
MPP-3958: Note that color audit is needed
jwhitlock Nov 12, 2024
cf181ba
MPP-3959: Note that updates are available
jwhitlock Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions .github/pull_request_template.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,30 +9,27 @@ How to test:
- [ ] l10n changes have been submitted to the l10n repository, if any.
- [ ] I've added a unit test to test for potential regressions of this bug.
- [ ] I've added or updated relevant docs in the docs/ directory.
- [ ] All UI revisions follow the [coding standards](https://github.com/mozilla/fx-private-relay/blob/main/docs/coding-standards.md), and use Protocol tokens where applicable (see `/frontend/src/styles/tokens.scss`).
- [ ] All UI revisions follow the [coding standards](https://github.com/mozilla/fx-private-relay/blob/main/docs/coding-standards.md), and use Protocol / Nebula colors where applicable (see `/frontend/src/styles/colors.scss`).
jwhitlock marked this conversation as resolved.
Show resolved Hide resolved
- [ ] Commits in this PR are minimal and [have descriptive commit messages](https://chris.beams.io/posts/git-commit/).

<!-- When adding a new feature: -->

# New feature description



# Screenshot (if applicable)

Not applicable.

# How to test



# Checklist (Definition of Done)

- [ ] Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
- [ ] Customer Experience team has seen or waived a demo of functionality.
- [ ] All acceptance criteria are met.
- [ ] Jira ticket has been updated (if needed) to match changes made during the development process.
- [ ] I've added or updated relevant docs in the docs/ directory
- [ ] Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.
- [ ] All UI revisions follow the [coding standards](https://github.com/mozilla/fx-private-relay/blob/main/docs/coding-standards.md), and use Protocol tokens where applicable (see `/frontend/src/styles/tokens.scss`).
- [ ] All UI revisions follow the [coding standards](https://github.com/mozilla/fx-private-relay/blob/main/docs/coding-standards.md), and use Protocol / Nebula colors where applicable (see `/frontend/src/styles/colors.scss`).
- [ ] Commits in this PR are minimal and [have descriptive commit messages](https://chris.beams.io/posts/git-commit/).
- [ ] l10n changes have been submitted to the l10n repository, if any.
4 changes: 0 additions & 4 deletions frontend/next.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,6 @@ module.exports = {
// TODO MPP-3946: Fix deprecation warnings in sass 1.80.x
// https://github.com/mozilla/protocol/releases/tag/v18.0.0
Comment on lines 118 to 119
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

silenceDeprecations: [
// Issues we can fix in our code
"import", // https://sass-lang.com/documentation/breaking-changes/import/
"mixed-decls", // https://sass-lang.com/d/mixed-decls

// Upstream issues
"legacy-js-api", // vercel/next.js issue #71638
],
Expand Down
40 changes: 20 additions & 20 deletions frontend/src/components/Banner.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
@import "~@mozilla-protocol/core/protocol/css/includes/lib";
@import "~@mozilla-protocol/core/protocol/css/includes/forms/lib";
@import "../styles/tokens";
@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
@use "~@mozilla-protocol/core/protocol/css/includes/forms/lib" as mp_forms;
@use "../styles/color";

.banner {
border-radius: $border-radius-md;
padding: $spacing-md;
background-color: $color-white;
background-color: color.$white;
position: relative;
box-shadow: $box-shadow-sm;

&.promo {
// This is the gradient border width (which is implemented as a background image):
padding: 2px;
box-shadow: none;
background-image: $firefoxGradient;
background-image: color.$firefoxGradient;
}

&.warning {
Expand Down Expand Up @@ -44,15 +44,15 @@
padding: 0;

&:hover {
background-color: $color-blue-50;
color: $color-white;
background-color: color.$blue-50;
color: color.$white;
}
}
}

.highlight-wrapper {
display: flex;
background-color: $color-white;
background-color: color.$white;
flex-wrap: wrap;

.banner.promo & {
Expand All @@ -75,7 +75,7 @@
padding-left: $spacing-md;
border-left-width: 4px;
border-left-style: solid;
border-color: $color-yellow-50;
border-color: color.$yellow-50;
}
}
}
Expand All @@ -90,7 +90,7 @@

.info-icon {
align-self: center;
color: $color-violet-30;
color: color.$violet-30;
}
}

Expand Down Expand Up @@ -154,11 +154,11 @@
margin-right: $spacing-sm;

.warning & {
color: $color-yellow-50;
color: color.$yellow-50;
}

.info & {
color: $color-violet-30;
color: color.$violet-30;
margin-right: $spacing-xs;
}
}
Expand All @@ -170,7 +170,7 @@
display: inline-block;
padding-top: $spacing-md;
font-weight: 700;
color: $color-blue-50;
color: color.$blue-50;
cursor: pointer;
background: none;
border: none;
Expand All @@ -182,7 +182,7 @@
}

&:hover {
color: $color-blue-40;
color: color.$blue-40;
}

&:focus {
Expand All @@ -207,18 +207,18 @@
display: block;
padding: $spacing-sm $spacing-md;
font-weight: 700;
border: 2px solid $color-blue-50;
color: $color-blue-50;
border: 2px solid color.$blue-50;
color: color.$blue-50;
border-radius: $border-radius-sm;

&:hover {
background-color: $color-blue-60;
color: $color-white;
background-color: color.$blue-60;
color: color.$white;
}

&:focus {
border-color: $button-border-color-focus;
box-shadow: $field-focus-ring;
border-color: mp_forms.$button-border-color-focus;
box-shadow: mp_forms.$field-focus-ring;
outline: none;
}
}
Expand Down
10 changes: 5 additions & 5 deletions frontend/src/components/Button.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
@import "../styles/tokens";
@import "~@mozilla-protocol/core/protocol/css/includes/mixins/typography";
@import "~@mozilla-protocol/core/protocol/css/includes/forms/lib";
@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
@use "~@mozilla-protocol/core/protocol/css/includes/forms/lib" as mp_forms;
@use "../styles/color";

.button {
display: inline-flex;
Expand All @@ -24,8 +24,8 @@
}

&:focus {
border-color: $button-border-color-focus;
box-shadow: $field-focus-ring;
border-color: mp_forms.$button-border-color-focus;
box-shadow: mp_forms.$field-focus-ring;
color: $color-white;
outline: none;
}
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/components/InfoModal.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
@import "../styles/tokens";
@import "~@mozilla-protocol/core/protocol/css/includes/lib";
@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
@use "../styles/color";

.underlay {
align-items: center;
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/components/InfoTooltip.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
@import "../styles/tokens";
@import "~@mozilla-protocol/core/protocol/css/includes/lib";
@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
@use "../styles/color";

.wrapper {
position: relative;
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/components/dashboard/CornerNotification.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
@import "../../styles/tokens";
@import "~@mozilla-protocol/core/protocol/css/includes/lib";
@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
@use "../../styles/color";

.wrapper {
position: relative;
Expand Down
26 changes: 14 additions & 12 deletions frontend/src/components/dashboard/EmailForwardingModal.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
@import "../../styles/tokens";
@import "~@mozilla-protocol/core/protocol/css/includes/lib";
@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
@use "../../styles/color";
@use "../../styles/text";

.underlay {
position: fixed;
Expand Down Expand Up @@ -64,20 +65,21 @@
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

display: flex;
align-items: center;
justify-content: center;
padding: $spacing-sm 0;
gap: $spacing-sm;
font-weight: 100;
}
}

.modal-title {
font-weight: 600;

@include text-body-md;
color: $color-grey-50;
color: color.$grey-50;
}

.nevermind-link {
Expand All @@ -95,14 +97,14 @@

.label-input {
font-family: $font-stack-firefox;
border: 1px solid $color-grey-30;
border: 1px solid color.$grey-30;
border-radius: $border-radius-sm;
font-weight: 100;
padding: $spacing-sm $spacing-md;

// Inputs should be atleast 16px if we want to avoid iOS auto-zooming (MPP-3598)
@include text-body-md;
color: $color-grey-40;
color: color.$grey-40;
width: 100%;
}

Expand Down
35 changes: 19 additions & 16 deletions frontend/src/components/dashboard/FreeOnboarding.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
@import "../../styles/tokens";
@import "~@mozilla-protocol/core/protocol/css/includes/lib";
@import "~@mozilla-protocol/core/protocol/css/includes/forms/lib";
@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
@use "~@mozilla-protocol/core/protocol/css/includes/forms/lib" as mp_forms;
@use "../../styles/color";
@use "../../styles/text";

.onboarding {
display: flex;
Expand Down Expand Up @@ -176,14 +177,15 @@

.domain-example {
@include font-firefox;
@include text-title-2xs;
color: $color-light-gray-90;
display: inline-block;
padding: $spacing-md 0;
text-overflow: ellipsis;
white-space: nowrap;
overflow-x: hidden;
max-width: $content-xs;
@include text.title-2xs {
color: $color-light-gray-90;
display: inline-block;
padding: $spacing-md 0;
text-overflow: ellipsis;
white-space: nowrap;
overflow-x: hidden;
max-width: $content-xs;
}

@media screen and #{$mq-md} {
max-width: 100%;
Expand All @@ -208,7 +210,7 @@
}

input {
@include form-input;
@include mp_forms.form-input;
flex-grow: 2;
margin: 0;
margin-bottom: $spacing-md;
Expand Down Expand Up @@ -273,10 +275,11 @@
gap: $spacing-xs;

h1 {
@include text-title-sm;
font-weight: 700;
font-family: $font-stack-firefox;
color: $color-purple-50;
@include text.title-sm {
font-weight: 700;
font-family: $font-stack-firefox;
color: $color-purple-50;
}
}

p {
Expand Down
12 changes: 7 additions & 5 deletions frontend/src/components/dashboard/Onboarding.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
@import "../../styles/tokens";
@import "~@mozilla-protocol/core/protocol/css/includes/lib";
@use "~@mozilla-protocol/core/protocol/css/includes/lib" as *;
@use "../../styles/color";
@use "../../styles/text";

.wrapper {
padding: $spacing-lg;
border-radius: $border-radius-md;
border: 1px solid $color-light-gray-40;

h2 {
@include text-title-2xs;
@include font-firefox;
padding: $spacing-sm 0;
@include text.title-2xs {
@include font-firefox;
padding: $spacing-sm 0;
}
}

.steps {
Expand Down
Loading