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

More concise improvements for add() and subtract() #34432

Merged
merged 5 commits into from
Jul 27, 2021

Conversation

ffoodd
Copy link
Member

@ffoodd ffoodd commented Jul 6, 2021

Better fix for #33953, improves #34047

@oliwerAR Back to this, made a quick SassMeister to play with.

From my understanding, brackets are required on subtract() second argument only when it contains an addition, am I right? Thinking so, I also checked that second argument contains a + sign to only output brackets when necessary.

@ffoodd ffoodd requested a review from a team as a code owner July 6, 2021 09:30
@oliwerAR
Copy link

oliwerAR commented Jul 6, 2021

@ffoodd I'm afraid that's not right. The second argument of substract should be put in brackets always when it's a complex expression (not type-of number), whatever sign it contains.

Simple match:
a - b = ?
a = 100

b = 10 + 1 = 11
100 - (10 + 1) = 100 - 10 - 1 = 89

b = 10 - 1 = 9
100 - (10 - 1) = 100 - 10 + 1 = 91

The results of your last two tests:

    font-size: subtract(1vw, $add); =  font-size: calc(1vw - (1rem + 1em));
    font-size: subtract(1vw, $subtract); =  font-size: calc(1vw - 1rem - 1em);

are mathematically equal, though obviously the should not be.

@ffoodd ffoodd force-pushed the main-fod-subtract-improvements branch from 24784ae to 4d19246 Compare July 6, 2021 10:20
@ffoodd
Copy link
Member Author

ffoodd commented Jul 6, 2021

Just updated the PR and made another SassMeister. Should be better :)

@oliwerAR
Copy link

oliwerAR commented Jul 7, 2021

@ffoodd This version seams to be working fine (mathematically correct).

@XhmikosR
Copy link
Member

XhmikosR commented Jul 8, 2021

Just to confirm @ffoodd did you check the dist file changes? Does everything look good now?

@ffoodd
Copy link
Member Author

ffoodd commented Jul 12, 2021

@XhmikosR Yes, we only had 5 occurrences of unneeded parenthesis ((x + (a + b)) for all of them). AFAIK it's safe for our core, and should be for other cases as demonstrated in Sassmeister.

@XhmikosR XhmikosR requested a review from mdo July 12, 2021 14:11
@ffoodd ffoodd changed the title refactor(functions): add() and subtract() more concise improvements More concise improvements for add() and subtract() Jul 12, 2021
@XhmikosR XhmikosR changed the title More concise improvements for add() and subtract() More concise improvements for add() and subtract() Jul 27, 2021
@XhmikosR XhmikosR merged commit 8536474 into main Jul 27, 2021
@XhmikosR XhmikosR deleted the main-fod-subtract-improvements branch July 27, 2021 04:48
marvin-hinkley-vortx pushed a commit to Vortx-Inc/bootstrap that referenced this pull request Aug 18, 2021
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.

4 participants