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 functions tests #1912

Merged
merged 13 commits into from
Aug 9, 2023
Merged

Calc functions tests #1912

merged 13 commits into from
Aug 9, 2023

Conversation

pamelalozano16
Copy link
Contributor

@pamelalozano16 pamelalozano16 commented May 19, 2023

Context: Calc functions proposal
[skip dart-sass]

@Goodwine
Copy link
Member

Nit, you want to add [skip dart-sass] in your PR description so the CI checks don't run the tests against the current dart sass version

You can also run npm run lint-spec -- --fix to automatically fix linter errors

spec/values/calculation/sqrt.hrx Outdated Show resolved Hide resolved
spec/values/calculation/sqrt.hrx Show resolved Hide resolved
Copy link
Member

@Goodwine Goodwine left a comment

Choose a reason for hiding this comment

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

LGTM but I'll leave it to Jen to do a final approval.

Note that if you get an approval, you should not submit until both PRs are approved since they should be merged roughly at the same time.

spec/values/calculation/sqrt.hrx Outdated Show resolved Hide resolved
Copy link
Member

@jathak jathak left a comment

Choose a reason for hiding this comment

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

Looking at the spec (both ours and the CSS spec), I realize now that I mistakenly had the impression (and then shared that with you) that these new math functions have to be contained within another calc to work (MDN was unhelpful here its examples for sqrt and most of the other functions did have them nested).

In actuality, code like

a {
  b: sqrt(4);
}

is valid and the sqrt(4) should simplify to 2.

Given that, most of these tests should just have sqrt without a calc wrapping them (maybe have one test like that to test the nesting). They otherwise look good (though I'll wait to give final approval until all of the functions are implemented/tested).

Sorry for the confusion!

@pamelalozano16 pamelalozano16 changed the title Sqrt calc function test Calc functions test May 19, 2023
@pamelalozano16 pamelalozano16 changed the title Calc functions test Calc functions tests May 19, 2023
@pamelalozano16 pamelalozano16 force-pushed the pamelalozano branch 10 times, most recently from 85c6a49 to d24402a Compare May 27, 2023 05:56
@pamelalozano16 pamelalozano16 force-pushed the pamelalozano branch 4 times, most recently from 26db9d2 to 05bcabe Compare June 2, 2023 17:44
spec/values/calculation/abs.hrx Show resolved Hide resolved
spec/values/calculation/mod.hrx Outdated Show resolved Hide resolved
spec/values/calculation/mod.hrx Outdated Show resolved Hide resolved
spec/values/calculation/round/strategies/down.hrx Outdated Show resolved Hide resolved
spec/values/calculation/round/strategies/down.hrx Outdated Show resolved Hide resolved
spec/values/calculation/round/strategies/down.hrx Outdated Show resolved Hide resolved
spec/values/calculation/round/two_arguments.hrx Outdated Show resolved Hide resolved
spec/values/calculation/round/two_arguments.hrx Outdated Show resolved Hide resolved
spec/values/calculation/round/error.hrx Outdated Show resolved Hide resolved
spec/values/calculation/round/one_argument.hrx Outdated Show resolved Hide resolved
spec/values/calculation/round/strategy/README.md Outdated Show resolved Hide resolved
spec/values/calculation/round/strategy/down.hrx Outdated Show resolved Hide resolved
spec/values/calculation/round/two_arguments.hrx Outdated Show resolved Hide resolved
@pamelalozano16 pamelalozano16 merged commit bac935b into main Aug 9, 2023
12 checks passed
@pamelalozano16 pamelalozano16 deleted the pamelalozano branch August 9, 2023 21:14
nex3 added a commit that referenced this pull request Aug 17, 2023
nex3 added a commit that referenced this pull request Aug 17, 2023
nex3 added a commit that referenced this pull request Sep 1, 2023
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.

4 participants