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

use dart sass, and update related modules #10208

Merged
merged 1 commit into from
Jan 19, 2021
Merged

use dart sass, and update related modules #10208

merged 1 commit into from
Jan 19, 2021

Conversation

brad-decker
Copy link
Contributor

@brad-decker brad-decker commented Jan 18, 2021

Rationale

At some point in the future, I would like to refactor the way we use our scss files to make use of the @use rule. switching from node-sass to sass is a requirement of doing that, because node-sass binds to libsass which hasn't received new language features since November 2018. The official implementation of sass is dart-sass which has js bindings through the sass dependency.

More short-termed: I have need of the math.pow function that was added officially to the math module but never landed in libsass

This PR

  1. removes top-level node-sass dependency (gulp-sass still depends on it and it must be compiled in prep-deps on circleci).
  2. adds sass (js bindings to dart-sass)
  3. adds fibers, which is recommended by both gulp-sass and sass-loader for improving performance during compilation. despite this recommendation, the fibers package is marked as deprecated and heavily discouraged from use. Furthermore, it didn't appear to do anything in terms of performance on the build. I removed this complexity from the PR.
  4. updates sass-loader
  5. passes sass as implementation to sass-loader
  6. updates design-system/index.scss for @forward its contents, which should have zero impact on the final result(1). However, if node-sass was still being used it would fail to compile.
  1. It did end up having an impact, apparently @forward doesn't forward maps? I'm not sure what happened there but in a future PR where I used $color-map from desing-system/colors it didn't work until I @use'd colors directly. This isn't blocking this PR because $color-map is not used yet.

@metamaskbot
Copy link
Collaborator

Builds ready [d14ec0c]
Page Load Metrics (481 ± 64 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaintNaNNaNNaNNaNNaN
domContentLoaded32877247913464
load33077348113464
domInteractive32877147913464

@brad-decker brad-decker marked this pull request as ready for review January 19, 2021 15:42
@brad-decker brad-decker requested review from kumavis and a team as code owners January 19, 2021 15:42
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Seems legit

@brad-decker brad-decker merged commit acbe38c into develop Jan 19, 2021
@brad-decker brad-decker deleted the update-sass branch January 19, 2021 16:54
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2021
@Gudahtt
Copy link
Member

Gudahtt commented Jan 19, 2021

Do any of these differences affect us?

@Gudahtt
Copy link
Member

Gudahtt commented Jan 19, 2021

The easiest way to tell might be to compare the .css output before and after this PR.

@brad-decker
Copy link
Contributor Author

@Gudahtt i'll do that, and report back -- i did do a number of things on the build presented by the bot in this PR to make sure there weren't glaring issues and nothing stuck out to me but I'll examine css output.

@brad-decker
Copy link
Contributor Author

the differences between the two files are plentiful, but they appear to be entirely changes in white space/formatting. Examples:

before this change:

    .tab-bar__tab__caret {
      display: none; }

      @media screen and (max-width: 575px) {
        .tab-bar__tab__caret {
          display: block;
          background-image: url("/images/caret-right.svg");
          width: 36px;
          height: 36px;
          opacity: 0.5;
          background-size: contain;
          background-repeat: no-repeat;
          background-position: center; }
          [dir='rtl'] .tab-bar__tab__caret {
            -webkit-transform: rotate(180deg);
                    transform: rotate(180deg); } }

 .transaction-activity-log__activity::after {
    content: '';
    position: absolute;
    left: 0;
    top: 0;
    height: 100%;
    width: 7px;
    border-right: 1px solid #909090; }

after this change:

.tab-bar__tab__caret {
  display: none;
}

@media screen and (max-width: 575px) {
  .tab-bar__tab__caret {
    display: block;
    background-image: url("/images/caret-right.svg");
    width: 36px;
    height: 36px;
    opacity: 0.5;
    background-size: contain;
    background-repeat: no-repeat;
    background-position: center;
  }
  [dir=rtl] .tab-bar__tab__caret {
    -webkit-transform: rotate(180deg);
            transform: rotate(180deg);
  }
}

.transaction-activity-log__activity::after {
  content: "";
  position: absolute;
  left: 0;
  top: 0;
  height: 100%;
  width: 7px;
  border-right: 1px solid #909090;
}

The only differences that concerned me were these:

.fa-stopwatch:before {
  content: "\f2f2"; }
.fa-stopwatch:before {
  content: "";
}

which after researching I found sass/dart-sass#568 -- I also double checked and the font-awesome fonts are still working on storybook and the extension.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants