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

calc() it up — use Sass calculations more #39606

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

wlib
Copy link

@wlib wlib commented Jan 25, 2024

Description

  • I replaced all Sass +, -, and * math operators with Sass calc()ulations
    • note that the output CSS is simplified at compile time if possible; the output css is slightly smaller
  • I removed calc() from the disallowed functions in stylelint
  • I added a with-value() function
    • it ensures that the value has a unit, for calc() with addition or subtraction with potentially null or unitless values like zero
    • it makes some of the source scss more verbose than add() and subtract() but it is more explicit and the output css is slightly more optimized

Motivation & Context

I described the full motivation in #39507.

Also, a nice side-effect here is that the code is slightly less tedious to convert to use css variables.

Type of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (non-breaking change)
  • Breaking change (fix or feature that would change existing functionality)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • My change introduces changes to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

#39507

@wlib wlib marked this pull request as ready for review January 25, 2024 02:44
@wlib wlib requested a review from a team as a code owner January 25, 2024 02:44
Replace Sass number operators with calculations for math when possible.
@mdo
Copy link
Member

mdo commented Jan 25, 2024

Without reviewing in depth yet, I'll just add that this is the way and I'm glad you proposed it. Unsure what version we'll do this in just yet. Thoughts on v5.4 @julien-deramond?

@@ -126,7 +126,7 @@

> .btn:not(:first-child),
> .btn-group:not(:first-child) {
margin-top: calc(#{$btn-border-width} * -1); // stylelint-disable-line function-disallowed-list
Copy link
Member

Choose a reason for hiding this comment

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

You'll have to interpolate the Sass variables across all of these calc() functions.

Copy link
Author

Choose a reason for hiding this comment

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

oof... it looks like that is a requirement for node-sass in the failed CI, which uses LibSass (both deprecated in favor of Dart Sass).

See https://sass-lang.com/documentation/syntax/special-functions/#element-progid-and-expression

LibSass, Ruby Sass, and versions of Dart Sass prior to 1.40.0 parse calc() as special syntactic function like element().
Dart Sass versions 1.40.0 and later parse calc() as a calculation.

Does Bootstrap support LibSass builds in v5? I know that it was internally dropped but I mean for when people import and build the Sass themselves.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, v5 still supports it. We'll drop it in v6 I imagine.

Copy link
Author

Choose a reason for hiding this comment

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

This would probably have to hold off until then, because the calc simplification only happens when the variables are not interpolated. Switching everything to calc would probably make the output too large in most cases.

Copy link
Member

Choose a reason for hiding this comment

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

We could actually consider dropping LibSass in v5.4 or like a special v5.5. We'll be moving onto v6 dev at some point but TBD on actual timeline.

Copy link
Member

Choose a reason for hiding this comment

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

I agree in terms of realistic timeline that we could consider dropping LibSass in v5.4/v5.5. I hope, however, that it wouldn't trigger a semver comments war.

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

Successfully merging this pull request may close these issues.

None yet

4 participants