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

Add specs for unit math with multiple units #1205

Merged
merged 1 commit into from
Jan 23, 2018
Merged

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Jan 23, 2018

Skip dart-sass
Skip ruby-sass

@xzyfer
Copy link
Contributor

xzyfer commented Jan 24, 2018

@nex3 the difference in ruby and dart output is significant, which output is correct for libsass?

@nex3
Copy link
Contributor Author

nex3 commented Jan 24, 2018

I think inspect() output should have some flexibility, since it's intended for debugging. What matters here is which numerator and denominator units exist, not what format they're emitted in for debugging.

@mgreter
Copy link
Contributor

mgreter commented Jan 27, 2018

@nex3 can you elaborate why the following:

.test {
  test1: inspect((23px/2fu/12emu/1.2gnu));
}

Results in:

.test {
  test1: 0.7986111111px/emu*fu*gnu;
}

Or will this change with sass/sass#2457 ?

I have no problem coming up with a mathematically correct version, but I have a hard time to preserve the same unit in the output as ruby sass and throwing errors for incompatible operations at the same time.

@mgreter
Copy link
Contributor

mgreter commented Jan 27, 2018

BTW. since you seem to have introduced exponent notation (^-1) for dart-sass, are there any plans to add this in a more generic fashion (as for number and/or units themselves)?

Another small issue I had with my implementation was that it initially converted the following:

.test {
  $nr: (1s*1000ms);
  test: inspect($nr);
}

To:

.test {
  test: 1s^2; }

Whereas ruby sass gives:

.test {
  test: 1000ms*s; }

Note: in order to preserve the same unit as ruby sass does, we need to postpone the number normalization in LibSass from after each operation step to when it is actually used. This will degrade performance a bit, since it means that we need i.e. to copy and normalize numbers each time we do a compare operation on numbers or other things that need the numbers normalized. Will make some profiling once it passes all tests again.

@mgreter
Copy link
Contributor

mgreter commented Jan 27, 2018

@nex3 here some other small things I discovered regarding units and numbers I would consider "wrong":

.test {
  fails: percentage(0.1%);
  works: nth(1 2 3, 2px);
  true: hue(hsla(120%, 20, 42%, 1)) == 120deg;
}

I would have expected to allow percentages anywhere unitless numbers are wanted (as in 120% = 1.2) and vice versa, but interestingly some functions (like hls) expect percentages but treats blank numbers like 10 equal to 10%. For me this seems quite confusing and somewhat missing in the documentation.

Must be between 0% and 100%, inclusive

@nex3
Copy link
Contributor Author

nex3 commented Jan 30, 2018

can you elaborate why the following:

.test {
  test1: inspect((23px/2fu/12emu/1.2gnu));
}

Results in:

.test {
  test1: 0.7986111111px/emu*fu*gnu;
}

Every time you divide by a (non-convertable) unit, that unit is added to the denominator units of the number. 23px / 2fu has numerator unit px and denominator unit fu; when you divide by 12emu, the denominator units are fu and emu; and when you divide by 1.2gnu, the denominator units are fu, emu, and gnu. The numerator units are placed on the left of the / and the denominator units are placed on the right; other than that, the order doesn't matter. So you get px/emu*fu*gnu.

BTW. since you seem to have introduced exponent notation (^-1) for dart-sass, are there any plans to add this in a more generic fashion (as for number and/or units themselves)?

No. Dart Sass just uses that notation because I thought it was clearer than /unit (and closer to what real-world unit arithmetic uses).

Another small issue I had with my implementation was that it initially converted the following:

.test {
  $nr: (1s*1000ms);
  test: inspect($nr);
}

To:

.test {
  test: 1s^2; }

Whereas ruby sass gives:

.test {
  test: 1000ms*s; }

I'd say both of these are correct. If normalizing eagerly here is easier for you, go for it.

here some other small things I discovered regarding units and numbers I would consider "wrong":

.test {
  fails: percentage(0.1%);
  works: nth(1 2 3, 2px);
  true: hue(hsla(120%, 20, 42%, 1)) == 120deg;
}

I would have expected to allow percentages anywhere unitless numbers are wanted (as in 120% = 1.2) and vice versa, but interestingly some functions (like hls) expect percentages but treats blank numbers like 10 equal to 10%. For me this seems quite confusing and somewhat missing in the documentation.

The percentage behavior here is unfortunately inherited from CSS. I agree that it would make sense to treat 120% as identical to 1.2, but that doesn't work well in practice. For example, CSS considers width: 50% to be different than width: 0.5.

We don't enforce unitless numbers in various built-in functions purely for backwards-compatibility reasons. Ideally they wouldn't be allowed, but it's not a big enough wart to be worth a deprecation process.

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.

3 participants