Skip to content

Build: Replace SASS division with function polyfill#4314

Merged
thisisdano merged 5 commits into
uswds:developfrom
aduth:aduth-sass-divide
Dec 13, 2021
Merged

Build: Replace SASS division with function polyfill#4314
thisisdano merged 5 commits into
uswds:developfrom
aduth:aduth-sass-divide

Conversation

@aduth

@aduth aduth commented Sep 3, 2021

Copy link
Copy Markdown
Contributor

Fixes #4204
Previously: #4209 (comment)

Description

This is an attempt to resolve SASS division deprecation warnings and errors in projects which use recent versions of Dart Sass. As an alternative to #4209, it is intended to be compatible across Dart Sass, LibSass, and Ruby Sass. It does this by avoiding the division operation altogether, generating an equivalent result with only multiplication, addition, and subtraction. This could serve as a temporary solution until support for LibSass and Ruby Sass are dropped.

Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fantastic, thanks for your help on this. Minor comment on placement on function and question on spacing-multiple().

Otherwise LGTM.

Comment thread src/stylesheets/core/_functions.scss Outdated
Comment thread src/stylesheets/core/_functions.scss Outdated
9: divide(100% * 9, 12),
10: divide(100% * 10, 12),
11: divide(100% * 11, 12),
12: divide(100% * 12, 12),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's just confirm this gives the expected results.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A small difference in that there's no rounding for the final precision value at 10 decimal places (e.g. 16.6666666667% vs. 16.6666666666%):

https://www.sassmeister.com/gist/aduth/bcfcde6eeeec8272e56c4321a6678f48

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It might be possible to emulate the rounding: Do one extra level of recursion past $precision, then + 1 the last digit if that value is >= 5. 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pushed the rounding changes in dad2a78.

To more accurately reflect an equivalent division value
A bit easier to follow the rounding logic
@aduth

aduth commented Nov 3, 2021

Copy link
Copy Markdown
Contributor Author

A couple additional thoughts as I've been thinking about this again lately:

  • Depending on what your schedule is for dropping legacy SASS support, this might not be worth it if old SASS support can be dropped outright. The advantage here is supporting both legacy SASS compilers as well as the Dart SASS compiler.
  • I expect the division function here would perform slower than a native division operation. Depending how much a concern this is, and how long it might need to linger around for, it could be worth changing some of the operations to some equivalent operation, such as using multiplication.
    • divide($theme-card-gap, 2) ➡️ $theme-card-gap * .5
  • In testing, there's a significant difference in compile time between legacy SASS compilers and Dart SASS, around 90% faster:
    • time dsass src/stylesheets/uswds.scss out.css # 1.18s user 0.14s system 101% cpu 1.295 total
    • time sass src/stylesheets/uswds.scss out.css # 18.07s user 0.40s system 99% cpu 18.582 total
  • This might not be the only deprecated feature that needs to be resolved before being compatible with Dart SASS. Specifically, there's also a warning about interpolation. This no longer logs when running Dart SASS, but I expect it's because the new behavior is taking effect:
    • DEPRECATION WARNING on line 55 of src/stylesheets/components/_icon-list.scss: #{} interpolation near operators will be simplified in a future version of Sass.

@aduth

aduth commented Nov 3, 2021

Copy link
Copy Markdown
Contributor Author

This might not be the only deprecated feature that needs to be resolved before being compatible with Dart SASS. Specifically, there's also a warning about interpolation. This no longer logs when running Dart SASS, but I expect it's because the new behavior is taking effect:

See also: #4380

@sean-usds

Copy link
Copy Markdown

@aduth thanks for fixing this! Just checking to see if/when this will get merged. Looking to clean up build warnings in a USDS/CDC project

@thisisdano thisisdano merged commit f65fc2d into uswds:develop Dec 13, 2021
@thisisdano

Copy link
Copy Markdown
Contributor

Will be released this week in USWDS 2.13.0

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.

Dart Sass deprecation warnings: slash as division

5 participants