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

Simplify responsive utilities with $responsive-variants #545

Merged
merged 33 commits into from
Oct 18, 2018

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Jul 18, 2018

This PR introduces a new variable in primer-support called $responsive-variants, which is a map:

$responsive-variants: (
  "": "",
  sm: "-sm",
  md: "-md",
  lg: "-lg",
  xl: "-xl"
);

The second thing I've done is modify the breakpoint() mixin so that if it gets an empty string it just outputs @content without a media query. This is not useful on its own, but makes it possible to replace:

.d-none { display: none !important; }

@each $breakpoint in map-keys($breakpoints) {
  .d-#{$breakpoint}-none { display: none !important; }
}

with:

@each $breakpoint, $variant in $responsive-variants {
  @include breakpoint($breakpoint) {
    .d#{$variant}-none { display: none !important; }
  }
}

The resulting CSS for the above examples should be the same. The trick is just that the first iteration in the @each loop sets $breakpoint and $variant to an empty string, which outputs the utility class without the media query. Each successive iteration outputs a responsive variation with -#{$breakpoint} substituted for #{$variant} in an @media block. I've added docs for this variable and pattern.

Also in this PR

  1. Put $responsive-variants to use for all of our existing responsive utilities: .float-*, .d-*, .border-*, .direction-*, .m-*, .p-*, .rounded-*, et al.
  2. Fix More granular border radius utilities #533 by adding (responsive!) edge-specific border radius utils: .rounded-top-1, .rounded-right-2, .rounded-bottom-0, etc. Adding corner-specific utilities would double the number of classes, so I think this is a good compromise.
  3. Fix Remove negative zero margin utilities #472 by nixing interpolations and wrapping the -n0 variants of our margin utilities in a conditional.
  4. Introduce a $display-values list that controls which .d-* classes get generated.
  5. Includes a fix for offset-* classes don't have !important #576 via Add responsive variants & !important to grid offset classes #582 ❤️ @sophshep

@@ -58,3 +58,29 @@ Media queries are scoped from each breakpoint and upwards. In the example below,
@include breakpoint(md) { font-size: 32px; }
}
```

## Responsive variants
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These docs should live in the contributing section of the style guide.

@shawnbot

This comment has been minimized.

@shawnbot
Copy link
Contributor Author

Well, good news! Our bundle is not significantly larger:

# https://github.com/sindresorhus/gzip-size-cli
% npm i -g gzip-size-cli
% curl -L https://unpkg.com/primer/build/build.css | gzip-size --raw
21531

% cd primer/primer/modules/primer
% npm run build # generates build/build.css
% gzip-size --raw build/build.css 
22050

In other words, from 21.5k to 22.1k, or a 0.6k difference.

@emplums
Copy link
Contributor

emplums commented Jul 23, 2018

Would it be possible to do a diff on the output just to make sure we aren't deleting any rules we don't want to delete?

@shawnbot
Copy link
Contributor Author

@emplums yep, let me see if I can do that.

@shawnbot
Copy link
Contributor Author

And here's the much cleaner diff after moving the responsive borders from primer-marketing-utilities into primer-utilities. I've included a diff of the size fields in the cssstats object too, just for fun.

Question for @sophshep: In strict semver terms, this change should trigger a major version bump to primer-marketing-utilities (and primer) because we're removing selectors. But I wonder if we need to do that because there probably aren't any projects that use primer-marketing-utilities but don't also use primer-utilities, right? 🤔

- Expected
+ Received

@@ -1,16 +1,16 @@
  Object {
-   "gzipSize": 21681,
-   "humanizedSize": "131kB",
-   "rules": 2166,
+   "gzipSize": 21915,
+   "humanizedSize": "133kB",
+   "rules": 2219,
    "selectors": Object {
-     "class": 2403,
+     "class": 2455,
      "id": 0,
      "pseudoClass": 238,
      "pseudoElement": 196,
      "specificity": Object {
-       "average": 12.900548159749412,
+       "average": 12.838128116609129,
        "max": 41,
      },
-     "total": 2554,
+     "total": 2607,
      "type": 305,
      "values": Array [
@@ -1305,5 +1305,4 @@
        ".mb-lg-8",
        ".mb-lg-9",
-       ".mb-lg-n0",
        ".mb-lg-n1",
        ".mb-lg-n2",
@@ -1325,5 +1324,4 @@
        ".mb-md-8",
        ".mb-md-9",
-       ".mb-md-n0",
        ".mb-md-n1",
        ".mb-md-n2",
@@ -1332,5 +1330,4 @@
        ".mb-md-n5",
        ".mb-md-n6",
-       ".mb-n0",
        ".mb-n1",
        ".mb-n2",
@@ -1352,5 +1349,4 @@
        ".mb-sm-8",
        ".mb-sm-9",
-       ".mb-sm-n0",
        ".mb-sm-n1",
        ".mb-sm-n2",
@@ -1372,5 +1368,4 @@
        ".mb-xl-8",
        ".mb-xl-9",
-       ".mb-xl-n0",
        ".mb-xl-n1",
        ".mb-xl-n2",
@@ -1412,5 +1407,4 @@
        ".ml-lg-5",
        ".ml-lg-6",
-       ".ml-lg-n0",
        ".ml-lg-n1",
        ".ml-lg-n2",
@@ -1426,5 +1420,4 @@
        ".ml-md-5",
        ".ml-md-6",
-       ".ml-md-n0",
        ".ml-md-n1",
        ".ml-md-n2",
@@ -1433,5 +1426,4 @@
        ".ml-md-n5",
        ".ml-md-n6",
-       ".ml-n0",
        ".ml-n1",
        ".ml-n2",
@@ -1447,5 +1439,4 @@
        ".ml-sm-5",
        ".ml-sm-6",
-       ".ml-sm-n0",
        ".ml-sm-n1",
        ".ml-sm-n2",
@@ -1461,5 +1452,4 @@
        ".ml-xl-5",
        ".ml-xl-6",
-       ".ml-xl-n0",
        ".ml-xl-n1",
        ".ml-xl-n2",
@@ -1482,5 +1472,4 @@
        ".mr-lg-5",
        ".mr-lg-6",
-       ".mr-lg-n0",
        ".mr-lg-n1",
        ".mr-lg-n2",
@@ -1496,5 +1485,4 @@
        ".mr-md-5",
        ".mr-md-6",
-       ".mr-md-n0",
        ".mr-md-n1",
        ".mr-md-n2",
@@ -1503,5 +1491,4 @@
        ".mr-md-n5",
        ".mr-md-n6",
-       ".mr-n0",
        ".mr-n1",
        ".mr-n2",
@@ -1517,5 +1504,4 @@
        ".mr-sm-5",
        ".mr-sm-6",
-       ".mr-sm-n0",
        ".mr-sm-n1",
        ".mr-sm-n2",
@@ -1531,5 +1517,4 @@
        ".mr-xl-5",
        ".mr-xl-6",
-       ".mr-xl-n0",
        ".mr-xl-n1",
        ".mr-xl-n2",
@@ -1564,5 +1549,4 @@
        ".mt-lg-8",
        ".mt-lg-9",
-       ".mt-lg-n0",
        ".mt-lg-n1",
        ".mt-lg-n2",
@@ -1584,5 +1568,4 @@
        ".mt-md-8",
        ".mt-md-9",
-       ".mt-md-n0",
        ".mt-md-n1",
        ".mt-md-n2",
@@ -1591,5 +1574,4 @@
        ".mt-md-n5",
        ".mt-md-n6",
-       ".mt-n0",
        ".mt-n1",
        ".mt-n2",
@@ -1611,5 +1593,4 @@
        ".mt-sm-8",
        ".mt-sm-9",
-       ".mt-sm-n0",
        ".mt-sm-n1",
        ".mt-sm-n2",
@@ -1631,5 +1612,4 @@
        ".mt-xl-8",
        ".mt-xl-9",
-       ".mt-xl-n0",
        ".mt-xl-n1",
        ".mt-xl-n2",
@@ -2207,4 +2187,76 @@
        ".rounded-1",
        ".rounded-2",
+       ".rounded-bottom-0",
+       ".rounded-bottom-1",
+       ".rounded-bottom-2",
+       ".rounded-left-0",
+       ".rounded-left-1",
+       ".rounded-left-2",
+       ".rounded-lg-0",
+       ".rounded-lg-1",
+       ".rounded-lg-2",
+       ".rounded-lg-bottom-0",
+       ".rounded-lg-bottom-1",
+       ".rounded-lg-bottom-2",
+       ".rounded-lg-left-0",
+       ".rounded-lg-left-1",
+       ".rounded-lg-left-2",
+       ".rounded-lg-right-0",
+       ".rounded-lg-right-1",
+       ".rounded-lg-right-2",
+       ".rounded-lg-top-0",
+       ".rounded-lg-top-1",
+       ".rounded-lg-top-2",
+       ".rounded-md-0",
+       ".rounded-md-1",
+       ".rounded-md-2",
+       ".rounded-md-bottom-0",
+       ".rounded-md-bottom-1",
+       ".rounded-md-bottom-2",
+       ".rounded-md-left-0",
+       ".rounded-md-left-1",
+       ".rounded-md-left-2",
+       ".rounded-md-right-0",
+       ".rounded-md-right-1",
+       ".rounded-md-right-2",
+       ".rounded-md-top-0",
+       ".rounded-md-top-1",
+       ".rounded-md-top-2",
+       ".rounded-right-0",
+       ".rounded-right-1",
+       ".rounded-right-2",
+       ".rounded-sm-0",
+       ".rounded-sm-1",
+       ".rounded-sm-2",
+       ".rounded-sm-bottom-0",
+       ".rounded-sm-bottom-1",
+       ".rounded-sm-bottom-2",
+       ".rounded-sm-left-0",
+       ".rounded-sm-left-1",
+       ".rounded-sm-left-2",
+       ".rounded-sm-right-0",
+       ".rounded-sm-right-1",
+       ".rounded-sm-right-2",
+       ".rounded-sm-top-0",
+       ".rounded-sm-top-1",
+       ".rounded-sm-top-2",
+       ".rounded-top-0",
+       ".rounded-top-1",
+       ".rounded-top-2",
+       ".rounded-xl-0",
+       ".rounded-xl-1",
+       ".rounded-xl-2",
+       ".rounded-xl-bottom-0",
+       ".rounded-xl-bottom-1",
+       ".rounded-xl-bottom-2",
+       ".rounded-xl-left-0",
+       ".rounded-xl-left-1",
+       ".rounded-xl-left-2",
+       ".rounded-xl-right-0",
+       ".rounded-xl-right-1",
+       ".rounded-xl-right-2",
+       ".rounded-xl-top-0",
+       ".rounded-xl-top-1",
+       ".rounded-xl-top-2",
        ".rule",
        ".rule::after",
@@ -2402,4 +2454,5 @@
        "50%",
        ":-ms-input-placeholder",
+       "::-ms-input-placeholder",
        "::-webkit-file-upload-button",
        "::-webkit-input-placeholder",
@@ -2571,4 +2624,4 @@
      ],
    },
-   "size": 134045,
+   "size": 136526,
  }

@shawnbot shawnbot changed the base branch from release-10.8.0 to release-10.9.0 October 5, 2018 23:47
@emplums emplums mentioned this pull request Oct 15, 2018
16 tasks
@sophshep
Copy link

This approach looks great.

I'm embarrassed that I think I added negative margin utilities with the value of zero 😂

there probably aren't any projects that use primer-marketing-utilities but don't also use primer-utilities, right?

I can't think of any, no.

Add responsive variants & `!important` to grid offset classes
@emilybrick
Copy link
Contributor

I think this looks great! Agree that the $responsive-variants usage should be included in the contributing docs.

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.

6 participants