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

Handle complex expressions in add() & subtract() #34047

Merged
merged 4 commits into from
May 25, 2021
Merged

Conversation

ffoodd
Copy link
Member

@ffoodd ffoodd commented May 20, 2021

Fixes #33953

I don't expect any regression, but who knows :D Need to consider about backporting this to v4, if this gets merged.


SassMeister demoing this: https://www.sassmeister.com/gist/daceb834e14e092919b5c6ebaedf317b

@ffoodd ffoodd requested a review from a team as a code owner May 20, 2021 14:59
@ffoodd
Copy link
Member Author

ffoodd commented May 21, 2021

Thanks @XhmikosR :)

@XhmikosR XhmikosR merged commit 50cf7f4 into main May 25, 2021
@XhmikosR XhmikosR deleted the main-fod-calc-functions branch May 25, 2021 05:23
@XhmikosR XhmikosR changed the title Handle complex expressions in add() & subtract() Handle complex expressions in add() & subtract() May 25, 2021
@oliwerAR
Copy link

Hi
I took a loot at the new version of the add and subtract functions (I opened the #33953 issue). I get the impression that you "overdid" the fixes.

@debug add(1px, add(3em, 4%, fasle));                                          
// calc(1px + (3em + 4%)) -- unnecessary inner brackets

@debug add(1px, subtract(3em, 4%, fasle));                                     
// calc(1px + (3em - 4%)) -- unnecessary inner brackets

@debug subtract(1px, add(3em, 4%, fasle));                                     
// calc(1px - (3em + 4%))

@debug subtract(1px, subtract(3em, 4%, fasle));                                
// calc(1px - (3em - 4%))

@debug add(add(1px, 2vh, false), add(3em, 4%, fasle));                         
// calc((1px + 2vh) + (3em + 4%)) -- unnecessary inner brackets (both pairs)

@debug add(subtract(1px, 2vh, false), add(3em, 4%, fasle));                    
// calc((1px - 2vh) + (3em + 4%)) -- unnecessary inner brackets (both pairs)

@debug add(add(1px, 2vh, false), subtract(3em, 4%, fasle));                    
// calc((1px + 2vh) + (3em - 4%)) -- unnecessary inner brackets (both pairs)

@debug add(subtract(1px, 2vh, false), subtract(3em, 4%, fasle));               
// calc((1px - 2vh) + (3em - 4%)) -- unnecessary inner brackets (both pairs)

@debug subtract(add(1px, 2vh, false), add(3em, 4%, fasle));                    
// calc((1px + 2vh) - (3em + 4%)) -- unnecessary first pair of inner brackets

@debug subtract(subtract(1px, 2vh, false), add(3em, 4%, fasle));               
// calc((1px - 2vh) - (3em + 4%)) -- unnecessary first pair of inner brackets

@debug subtract(add(1px, 2vh, false),subtract(3em, 4%, fasle));                
// calc((1px + 2vh) - (3em - 4%)) -- unnecessary first pair of inner brackets

@debug subtract(subtract(1px, 2vh, false),subtract(3em, 4%, fasle));          
 // calc((1px - 2vh) - (3em - 4%)) -- unnecessary first pair of inner brackets

In other words the add function doesn't need the inner brackets at all.
The subtract function only needs it on the second argument. All other inner brackets just add unnecessary, useless code.

@XhmikosR
Copy link
Member

@ffoodd can you have a look at the above comment please?

@ffoodd
Copy link
Member Author

ffoodd commented Jun 16, 2021

Back to this, I'm opened for suggestion @oliwerAR but I don't think there's a way to both solve the initial issue and prevent unnecessary brackets. This is not overdone since it's the most basic check we can do: number or not, and if not wrap into brackets.

Tell me if I'm wrong but all of your examples are working fine, aren't they?

@oliwerAR
Copy link

@ffoodd Yes, the examples are working but most of the brackets are useless.
My point (and the examples above) is, the only necessary place you need to check and add brackets is for the second argument of the subtract function - correct me if my math is wrong.
Doing all other checks and possibly adding brackets for the rest doesn't do anything (regardless of the type of the argument) except generating unnecessary characters in the output.
In other words it probably should look like this:

@function add($value1, $value2, $return-calc: true) {
  @if $value1 == null {
    @return $value2;
  }

  @if $value2 == null {
    @return $value1;
  }

  @if type-of($value1) == number and type-of($value2) == number and comparable($value1, $value2) {
    @return $value1 + $value2;
  }

  @return if($return-calc == true, calc(#{$value1} + #{$value2}), $value1 + unquote(" + ") + $value2);
}

@function subtract($value1, $value2, $return-calc: true) {
  @if $value1 == null and $value2 == null {
    @return null;
  }

  @if $value1 == null {
    @return -$value2;
  }

  @if $value2 == null {
    @return $value1;
  }

  @if type-of($value1) == number and type-of($value2) == number and comparable($value1, $value2) {
    @return $value1 - $value2;
  }

  @if type-of($value2) != number {
    $value2: unquote("(") + $value2 + unquote(")");
  }

  @return if($return-calc == true, calc(#{$value1} - #{$value2}), $value1 + unquote(" - ") + $value2);
}

@ffoodd
Copy link
Member Author

ffoodd commented Jun 17, 2021

So basically only keeping subtract()'s 2nd argument type check? I'll give it a shot and try to check results in Sassmeister.

@oliwerAR
Copy link

So basically only keeping subtract()'s 2nd argument type check? I'll give it a shot and try to check results in Sassmeister.

@ffoodd Exactly. It's the only thing that is negated, changes the sign - that was the problem in the initial issue.

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

Successfully merging this pull request may close these issues.

subtract add don't act as expected in some cases
4 participants