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

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

Closed
wants to merge 1 commit into from

Conversation

jwhitlock
Copy link
Member

@jwhitlock jwhitlock commented Nov 4, 2024

This PR addresses the Sass 1.80 deprecation warning about mixed declarations. It adds duplicate versions of @mozilla-protocol/[email protected] typography mixins to /frontend/src/styles/typography.scss that take a content block.

This code fragment raises a warning:

@import "../../styles/tokens";
@import "~@mozilla-protocol/core/protocol/css/includes/lib";

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

The warning:

Deprecation Warning on line ...
Sass's behavior for declarations that appear after nested
rules will be changing to match the behavior specified by CSS in an upcoming
version. To keep the existing behavior, move the declaration above the nested
rule. To opt into the new behavior, wrap the declaration in & {}.

More info: https://sass-lang.com/d/mixed-decls

This changed code eliminates the warning and results in identical CSS:

@import "../../styles/tokens";
@import "~@mozilla-protocol/core/protocol/css/includes/lib";
@import "../../styles/typography";

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

See below for more context on the issue and solution.

How to test

Similar to PR #5166, my goal was to eliminate the warning without changing the generated CSS. When reviewing, you may want to ignore whitespace, to see that the @include text-* contents were indented and not changed.

Run cd frontend; npm run build.

  • There are no Sass deprecation warnings
  • These file are still generated in /frontend/out/_next/static/css, which means the contents are identical to the previous versions:
    • 135df0ac639034c3.css
    • 4e63d6fb12ba45b3.css
    • 58e1c80c0e503be3.css
    • 644d429e5da963c9.css
    • 7830c6fa1d166113.css
    • 794f7edf20c99f0e.css
    • 7b7332110014f2b1.css
    • bcb82ac07001d0c5.css
    • c34d49dcc57a01db.css
    • fce17def8370eee1.css

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

Sass Mixed Declarations Deprecation

Our code triggers this warning with this pattern, from /frontend/src/components/dashboard/EmailForwardingModal.modules.scss:

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

This uses the text-title-xs mixin from @mozilla-protocol/[email protected], with this source:

https://github.com/mozilla/protocol/blob/f318aafa0f3b5ff8815c4b859d5a2de9146657f4/assets/sass/protocol/includes/mixins/_typography.scss#L100-L108

@mixin text-title-xs {
    @include font-size(type-scale('title-2xs-size'));
    line-height: type-scale('title-2xs-line-height');

    @media #{$mq-md} {
        @include font-size(type-scale('title-xs-size'));
        line-height: type-scale('title-xs-line-height');
    }
}

which is compiled to the (compressed) CSS:

.EmailForwardingModal_headline__VgCSC {
  /* The text-title-xs ruleset */
  font-size: 24px;
  font-size: 1.5rem;
  line-height: 1.08;
  /* The rest of the .headline properties */
  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
  }
}

In the future, Dart Sass will follow CSS's conventions, disallowing this form or turning it this into something like:

/* The text-title-xs ruleset */
.EmailForwardingModal_headline__VgCSC {
  font-size: 24px;
  font-size: 1.5rem;
  line-height: 1.08;
}
@media(min-width:768px)
{
  .EmailForwardingModal_headline__VgCSC {
    font-size: 28px;
    font-size: 1.75rem;
    line-height: 1.07
  }
/* The rest of the .headline properties as a second ruleset */
}
.EmailForwardingModal_headline__VgCSC {
  display: flex;
  align-items: center;
  justify-content: center;
  padding: 8px 0;
  gap: 8px;
  font-weight: 100
}

This may have consequences for our content. For example, the font-weight: 100 may not be applied on desktop-sized displayed.

The change is to create a new version of the @mixin that allows injecting our CSS properties before the @media nested rule:

@mixin text-title-xs {
    @include font-size(type-scale('title-2xs-size'));
    line-height: type-scale('title-2xs-line-height');
    @content;  // <-- Here is the added line

    @media #{$mq-md} {
        @include font-size(type-scale('title-xs-size'));
        line-height: type-scale('title-xs-line-height');
    }
}

These @mixin overrides are in /frontend/src/styles/typography.scss, along with a long explanatory comment. This allows minimal changes to eliminate the warning for the same CSS

  1. Add @include "path/to/styles/typography"; after @include "~@mozilla-protocol/core/protocol/css/includes/lib"; or similar
  2. Nest the properties as a content block for the @include text-* declaration

For example:

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

Future work

Relay is currently on @mozilla-protocol/[email protected], well behind the current version v19.2.0. Upgrading to future versions will require copying and modifying the new typography mixins.

Bedrock also has this issue (mozilla/bedrock#15117), but on @mozilla-protocol/[email protected]. They may solve the issue in a similar manner, find a different solution, or even create an upstream fix in mozilla-protocol/core. We can adopt their solution and eliminate this one.

An experienced CSS writer may use a different version of this fix. For example, this may be equivalent in our supported browsers:

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

It may better communicate the intent "Use the protocol extra-small title style, but with a different font weight". However, I am not an experienced CSS writer, so my guiding principle was "no CSS changes".

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/
@jwhitlock
Copy link
Member Author

Closing in favor of #5176

@jwhitlock jwhitlock closed this Nov 6, 2024
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.

1 participant