Skip to content

Use postcss-calc to simplify css calc(), closes #23581#23582

Closed
bangzek wants to merge 2 commits intotwbs:v4-devfrom
bangzek:postcss-calc
Closed

Use postcss-calc to simplify css calc(), closes #23581#23582
bangzek wants to merge 2 commits intotwbs:v4-devfrom
bangzek:postcss-calc

Conversation

@bangzek
Copy link
Copy Markdown

@bangzek bangzek commented Aug 21, 2017

It will simplify css calc() expression on .col-form-label, .col-form-label-lg and .col-form-label-sm.

@XhmikosR XhmikosR self-assigned this Aug 21, 2017
@XhmikosR
Copy link
Copy Markdown
Member

@bangzek: you need to update package-lock.json too.

@mdo: this is what I meant we should use more postcss plugins. This is an easy optimization, same as the mqpacker one. :)

@XhmikosR XhmikosR requested review from Johann-S and mdo August 21, 2017 09:39
@mdo
Copy link
Copy Markdown
Member

mdo commented Aug 21, 2017

This is an easy optimization, same as the mqpacker one. :)

Why do we need it?

@XhmikosR
Copy link
Copy Markdown
Member

XhmikosR commented Aug 21, 2017 via email

@mdo
Copy link
Copy Markdown
Member

mdo commented Aug 22, 2017

Okay, I think maybe I need to rephrase myself with these kind of questions. Here's what goes through my mind with PRs like this:

  • What does postcss-calc do?
  • How is it better or simpler than CSS's regular calc?
  • Is it more approachable than the regular calc?
  • Does it add more burden to us, or users of Bootstrap?
  • Why so many PostCSS plugins lately? We use Sass—how do we communicate this to folks?

None of these PRs lately have answers those questions @XhmikosR. You're telling me it's an optimization and it's better—you gotta tell me how exactly. I can go hunt some of this information down myself, but without it, I can't approve of the changes.

@XhmikosR
Copy link
Copy Markdown
Member

XhmikosR commented Aug 22, 2017

postcss-calc just reduces the calc references. In our case it's only 3 but it's a pretty safe and easy optimization we could/should have in our toolbox.

The resulting CSS has still the same effect. See the diff:

 dist/css/bootstrap.css | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/dist/css/bootstrap.css b/dist/css/bootstrap.css
index 9164d6876..8e667f466 100644
--- a/dist/css/bootstrap.css
+++ b/dist/css/bootstrap.css
@@ -1796,20 +1796,20 @@ select.form-control:focus::-ms-value {
 }
 
 .col-form-label {
-  padding-top: calc(0.5rem - 1px * 2);
-  padding-bottom: calc(0.5rem - 1px * 2);
+  padding-top: calc(0.5rem - 2px);
+  padding-bottom: calc(0.5rem - 2px);
   margin-bottom: 0;
 }
 
 .col-form-label-lg {
-  padding-top: calc(0.5rem - 1px * 2);
-  padding-bottom: calc(0.5rem - 1px * 2);
+  padding-top: calc(0.5rem - 2px);
+  padding-bottom: calc(0.5rem - 2px);
   font-size: 1.25rem;
 }
 
 .col-form-label-sm {
-  padding-top: calc(0.25rem - 1px * 2);
-  padding-bottom: calc(0.25rem - 1px * 2);
+  padding-top: calc(0.25rem - 2px);
+  padding-bottom: calc(0.25rem - 2px);
   font-size: 0.875rem;
 }

We do use Sass, but that doesn't mean we can't hook up Postcss plugins. We should definitely do this because Postcss is a post-processor not a pre-processor and can help a lot if we set up things carefully.

Note that one thing people blame us is the bloat/result size. We can't do miracles and keep the same functionality but we can squash a few KB here and there.

@mdo
Copy link
Copy Markdown
Member

mdo commented Aug 22, 2017

Ahhh okay so it rewrites things for a simpler output. That's chill. I didn't know if it was just affecting compiled code or trying to change how we use calc in our source Sass files.

@XhmikosR
Copy link
Copy Markdown
Member

@mdo: I think we can drop this PR since the gain isn't big.

@XhmikosR XhmikosR closed this Sep 15, 2017
@bangzek
Copy link
Copy Markdown
Author

bangzek commented Oct 4, 2017

This no longer needed cause that calc() on .col-form-label* not exist anymore

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.

4 participants