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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define proper breakpoint definitions rather then typing @media #4860

Merged
merged 11 commits into from Nov 4, 2018

Conversation

Projects
None yet
3 participants
@jjanssen
Member

jjanssen commented Oct 27, 2018

I decided to implement a proper mixin for mediaquery handling (based upon the thoughts of the fine people behind bootstrap). I think this would be a nice first step in the (hopefully) short-term fixing the mixed breakpoint usage.

In the morning I will perform a diff on the generated CSS, because essentially nothing should have been changed in the output 馃憤

  • Last check on CSS output in a file-diff

@jjanssen jjanssen force-pushed the jjanssen:feature/enable-breakpoint-mixins branch from e58516a to a09b8b5 Oct 28, 2018

@jjanssen

This comment has been minimized.

Member

jjanssen commented Oct 28, 2018

I did a final check on the before and after changes and I have 4 selectors with an alternate definition. Which are the breakpoints for targeting the smallest viewport and actually solved some overlapping a quirky behaviour 馃帀

Change 1:
image
Change 2:
image
Change 3:
image
Change 4:
image

The main difference in this breakpoint definition is that it no longer overlaps.
Previously the breakpoint mechanism always targeted going up and down with the same definition.

In a nutshell the following overlaps:

@media (max-width:50em) {}  // 800px
@media (min-width:50em) {}  // 800px

In the current changes this will result in the following:

@media (max-width:49.9375em) {}  // 799px
@media (min-width:50em) {}  // 800px

These actually solves the following visual quirks:
801px
image
800px
Current quirk:
image
With the changes 馃帀:
image
799px
image




@jjanssen jjanssen requested a review from thibaudcolas Oct 28, 2018

@jjanssen

This comment has been minimized.

Member

jjanssen commented Oct 28, 2018

Different quirk with tabs is fixed as well 馃槃 Both on 800px.

Before
image

After
image

$breakpoint-small: $breakpoint-mobile - 0.0625em;
$breakpoint-medium: $breakpoint-mobile;
$breakpoint-small: breakpoint-max(xs);
$breakpoint-medium: breakpoint-min(sm);
@mixin small {

This comment has been minimized.

@jjanssen

jjanssen Oct 28, 2018

Member

An after-thought maybe we should replace these mixins with the new introduced mixins. What are you thoughts on this?

This comment has been minimized.

@thibaudcolas

This comment has been minimized.

@jjanssen

jjanssen Nov 2, 2018

Member

In addition I also renamed and check the utility and visibility classes to have a consistent naming with the breakpoints. So @small became @xs and @medium became @sm.

@media screen and (min-width: $width-1440px) {
width: $width-1440px;
@include media-breakpoint-up(xl) {
width: breakpoint-min(xl);

This comment has been minimized.

@thibaudcolas

thibaudcolas Nov 1, 2018

Member

This seems to be the only place where the xl breakpoint is used. Should we re-do this with either lg or xxl?

This comment has been minimized.

@jjanssen

jjanssen Nov 2, 2018

Member

Ironic: it turns out it's not even called since the following is since the following from sm get applied.

image

$breakpoint-small: $breakpoint-mobile - 0.0625em;
$breakpoint-medium: $breakpoint-mobile;
$breakpoint-small: breakpoint-max(xs);
$breakpoint-medium: breakpoint-min(sm);
@mixin small {

This comment has been minimized.

@thibaudcolas

@jjanssen jjanssen force-pushed the jjanssen:feature/enable-breakpoint-mixins branch from d8cd4c1 to 89ae91b Nov 2, 2018

@jjanssen

This comment has been minimized.

Member

jjanssen commented Nov 2, 2018

All done and double-checked the CSS output difference with the current master. There are no notable difference aside from the previously accidentally fixed quirks.

@tomdyson

This comment has been minimized.

Contributor

tomdyson commented Nov 2, 2018

This looks really good, @jjanssen! Thanks a lot for all your excellent work on this.

@thibaudcolas

thibaudcolas requested changes Nov 4, 2018 edited

@jjanssen I have spotted some differences of targeted widths in the built CSS. I think they are due to how the new "down" mixin operates 鈥撀爄t includes the targeted breakpoint. I'll have a look at fixing those hopefully later today, should just be matter of lowering the problematic ones by one breakpoint.

Not sure what are your thoughts on this but in my opinion we shouldn't allow max-width queries to star with (mobile-first min-width only). It's a tough one to refactor though.

Here are the ones:

git diff -U2 --no-index --no-color a/wagtail-master/admin/static/wagtailadmin/css/layouts/404.css b/wagtail/admin/static/wagtailadmin/css/layouts/404.css
@@ -51,10 +51,10 @@ a.button.page404__button:hover {
     background-color: #007d7e;
 }
-@media screen and (max-width: 56.25em) {
+@media screen and (max-width: 74.9375em) {
     .wagtail-logo-container__desktop {
         display: none;
     }
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     .page404__text-container {
         width: 400px;
git diff -U2 --no-index --no-color a/wagtail-master/admin/static/wagtailadmin/css/layouts/home.css b/wagtail/admin/static/wagtailadmin/css/layouts/home.css
@@ -8,5 +8,5 @@ header .col1 {
     padding: 0;
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     header .col1 {
         position: relative;
@@ -69,15 +69,15 @@ header h2 {
     float: left;
 }
-@media screen and (max-width: 75em) {
+@media screen and (max-width: 99.9375em) {
     .summary ul.stats li:before {
         font-size: 5em;
     }
 }
-@media screen and (max-width: 56.25em) {
+@media screen and (max-width: 74.9375em) {
     .summary ul.stats li:before {
         font-size: 4.5em;
     }
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     .summary ul.stats li:before {
         font-size: 4em;
@@ -97,15 +97,15 @@ header h2 {
     font-weight: 300;
 }
-@media screen and (max-width: 75em) {
+@media screen and (max-width: 99.9375em) {
     .summary ul.stats span {
         font-size: 3em;
     }
 }
-@media screen and (max-width: 56.25em) {
+@media screen and (max-width: 74.9375em) {
     .summary ul.stats span {
         font-size: 2.5em;
     }
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     .summary ul.stats span {
         font-size: 2em;
git diff -U2 --no-index --no-color a/wagtail-master/admin/static/wagtailadmin/css/layouts/page-editor.css b/wagtail/admin/static/wagtailadmin/css/layouts/page-editor.css
@@ -454,5 +454,5 @@ body.ready .stream-menu button {
     padding-left: 3.1em;
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     .page-editor .header-title {
         padding-left: 0;
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     footer .preview .button,
     footer .preview button {

Full diff, & methodology:

ls wagtail/*/static/*/css/**/*.css

# On master
npm run dist
prettier --write wagtail/**/*.css
cp wagtail wagtail-master

# Check out target branch
git checkout <branch>
npm run dist
prettier --write wagtail/**/*.css

touch diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/core.css wagtail/admin/static/wagtailadmin/css/core.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/layouts/404.css wagtail/admin/static/wagtailadmin/css/layouts/404.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/layouts/compare-revisions.css wagtail/admin/static/wagtailadmin/css/layouts/compare-revisions.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/layouts/home.css wagtail/admin/static/wagtailadmin/css/layouts/home.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/layouts/login.css wagtail/admin/static/wagtailadmin/css/layouts/login.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/layouts/page-editor.css wagtail/admin/static/wagtailadmin/css/layouts/page-editor.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/normalize.css wagtail/admin/static/wagtailadmin/css/normalize.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/panels/draftail.css wagtail/admin/static/wagtailadmin/css/panels/draftail.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/panels/hallo.css wagtail/admin/static/wagtailadmin/css/panels/hallo.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/userbar.css wagtail/admin/static/wagtailadmin/css/userbar.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/vendor/jquery-ui/jquery-ui-1.10.3.verdant.css wagtail/admin/static/wagtailadmin/css/vendor/jquery-ui/jquery-ui-1.10.3.verdant.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/vendor/jquery.tagit.css wagtail/admin/static/wagtailadmin/css/vendor/jquery.tagit.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/admin/static/wagtailadmin/css/vendor/tagit.ui-zendesk.css wagtail/admin/static/wagtailadmin/css/vendor/tagit.ui-zendesk.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/documents/static/wagtaildocs/css/add-multiple.css wagtail/documents/static/wagtaildocs/css/add-multiple.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/images/static/wagtailimages/css/add-multiple.css wagtail/images/static/wagtailimages/css/add-multiple.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/images/static/wagtailimages/css/focal-point-chooser.css wagtail/images/static/wagtailimages/css/focal-point-chooser.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/images/static/wagtailimages/css/vendor/jquery.Jcrop.min.css wagtail/images/static/wagtailimages/css/vendor/jquery.Jcrop.min.css >> diffs.md
git diff -U2 --no-index --no-color wagtail-master/users/static/wagtailusers/css/groups_edit.css wagtail/users/static/wagtailusers/css/groups_edit.css >> diffs.md
git diff -U2 --no-index --no-color a/wagtail-master/admin/static/wagtailadmin/css/core.css b/wagtail/admin/static/wagtailadmin/css/core.css
index 198a11995..434ed25f3 100644
--- a/wagtail-master/admin/static/wagtailadmin/css/core.css
+++ b/wagtail/admin/static/wagtailadmin/css/core.css
@@ -583,5 +583,5 @@ body.ready .tab-nav a {
     }
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 49.9375em) {
     .tab-nav li:first-of-type {
         padding-left: 1.6em;
@@ -3099,5 +3099,5 @@ header.tab-merged {
     border: 0;
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 49.9375em) {
     header.no-border,
     header.tab-merged {
@@ -3208,5 +3208,5 @@ header .field-content {
     }
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 49.9375em) {
     .header-title {
         padding-left: 50px;
@@ -3558,5 +3558,5 @@ body.ready .progress .bar {
     background: #333;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .nav-wrapper .inner {
         display: flex;
@@ -3648,5 +3648,5 @@ body.ready .nav-main a {
     display: none;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .submenu-trigger:after {
         content: "n";
@@ -4318,5 +4318,5 @@ body.explorer-open .nav-main {
     background: rgba(0, 0, 0, 0.425);
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .c-explorer__item__link {
         padding: 1.45em 1.75em;
@@ -4383,5 +4383,5 @@ body.explorer-open .nav-main {
     flex-direction: column;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .explorer {
         width: 485px;
@@ -4398,5 +4398,5 @@ body.explorer-open .nav-main {
     background: #4c4e4d;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .c-explorer {
         box-shadow: 2px 2px 5px rgba(0, 0, 0, 0.425);
@@ -4423,5 +4423,5 @@ body.explorer-open .nav-main {
     color: #a5a5a5;
 }
-@media only screen and (max-width: 49.9375em) {
+@media screen and (max-width: 49.9375em) {
     .explorer-open .c-explorer__close {
         display: block;
@@ -4461,5 +4461,5 @@ body.explorer-open .nav-main {
     font-size: 1rem;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .c-explorer__header__inner {
         padding: 1em 1.5em;
@@ -4470,5 +4470,5 @@ body.explorer-open .nav-main {
     color: #fff;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .c-explorer__placeholder {
         padding: 1em 1.75em;
@@ -4492,5 +4492,5 @@ body.explorer-open .nav-main {
     background: rgba(0, 0, 0, 0.425);
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .c-explorer__see-more {
         padding: 1em 1.75em;
@@ -4501,31 +4501,31 @@ body.explorer-open .nav-main {
     display: none !important;
 }
-@media only screen and (min-width: 50em) {
-    .u-hidden\@medium {
+@media screen and (min-width: 50em) {
+    .u-hidden\@sm {
         display: none !important;
     }
 }
-@media only screen and (max-width: 49.9375em) {
-    .u-hidden\@small {
+@media screen and (max-width: 49.9375em) {
+    .u-hidden\@xs {
         display: none !important;
     }
 }
-@media only screen and (min-width: 50em) {
-    .u-inline\@medium {
+@media screen and (min-width: 50em) {
+    .u-inline\@sm {
         display: inline !important;
     }
 }
-@media only screen and (max-width: 49.9375em) {
-    .u-inline\@small {
+@media screen and (max-width: 49.9375em) {
+    .u-inline\@xs {
         display: inline !important;
     }
 }
-@media only screen and (min-width: 50em) {
-    .u-block\@medium {
+@media screen and (min-width: 50em) {
+    .u-block\@sm {
         display: block !important;
     }
 }
-@media only screen and (max-width: 49.9375em) {
-    .u-block\@small {
+@media screen and (max-width: 49.9375em) {
+    .u-block\@xs {
         display: block !important;
     }
@@ -4718,5 +4718,5 @@ footer .meta a:hover {
     color: #007d7e;
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 49.9375em) {
     footer .actions,
     footer .preview,
@@ -4732,9 +4732,4 @@ footer .meta a:hover {
     }
 }
-@media screen and (min-width: 90em) {
-    footer {
-        width: 90em;
-    }
-}
 .breadcrumb {
     margin-top: 0;
git diff -U2 --no-index --no-color a/wagtail-master/admin/static/wagtailadmin/css/layouts/404.css b/wagtail/admin/static/wagtailadmin/css/layouts/404.css
index 31d286996..82973f45c 100644
--- a/wagtail-master/admin/static/wagtailadmin/css/layouts/404.css
+++ b/wagtail/admin/static/wagtailadmin/css/layouts/404.css
@@ -51,10 +51,10 @@ a.button.page404__button:hover {
     background-color: #007d7e;
 }
-@media screen and (max-width: 56.25em) {
+@media screen and (max-width: 74.9375em) {
     .wagtail-logo-container__desktop {
         display: none;
     }
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     .page404__text-container {
         width: 400px;
git diff -U2 --no-index --no-color a/wagtail-master/admin/static/wagtailadmin/css/layouts/home.css b/wagtail/admin/static/wagtailadmin/css/layouts/home.css
index 8eb6ec64e..3aeb22731 100644
--- a/wagtail-master/admin/static/wagtailadmin/css/layouts/home.css
+++ b/wagtail/admin/static/wagtailadmin/css/layouts/home.css
@@ -8,5 +8,5 @@ header .col1 {
     padding: 0;
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     header .col1 {
         position: relative;
@@ -69,15 +69,15 @@ header h2 {
     float: left;
 }
-@media screen and (max-width: 75em) {
+@media screen and (max-width: 99.9375em) {
     .summary ul.stats li:before {
         font-size: 5em;
     }
 }
-@media screen and (max-width: 56.25em) {
+@media screen and (max-width: 74.9375em) {
     .summary ul.stats li:before {
         font-size: 4.5em;
     }
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     .summary ul.stats li:before {
         font-size: 4em;
@@ -97,15 +97,15 @@ header h2 {
     font-weight: 300;
 }
-@media screen and (max-width: 75em) {
+@media screen and (max-width: 99.9375em) {
     .summary ul.stats span {
         font-size: 3em;
     }
 }
-@media screen and (max-width: 56.25em) {
+@media screen and (max-width: 74.9375em) {
     .summary ul.stats span {
         font-size: 2.5em;
     }
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     .summary ul.stats span {
         font-size: 2em;
git diff -U2 --no-index --no-color a/wagtail-master/admin/static/wagtailadmin/css/layouts/page-editor.css b/wagtail/admin/static/wagtailadmin/css/layouts/page-editor.css
index 25a0f9cec..88eefbab1 100644
--- a/wagtail-master/admin/static/wagtailadmin/css/layouts/page-editor.css
+++ b/wagtail/admin/static/wagtailadmin/css/layouts/page-editor.css
@@ -433,5 +433,5 @@ body.ready .stream-menu button {
     margin: -1.2em 0 2em;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .page-editor .breadcrumb {
         margin-top: -1.8em;
@@ -454,5 +454,5 @@ body.ready .stream-menu button {
     padding-left: 3.1em;
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     .page-editor .header-title {
         padding-left: 0;
@@ -465,5 +465,5 @@ body.ready .stream-menu button {
     position: relative;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .object {
         padding-left: 50px;
@@ -588,5 +588,5 @@ body.ready .stream-menu button {
     padding-bottom: 2em;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .object.full .error-message {
         padding-left: 50px;
@@ -698,5 +698,5 @@ body.ready .object.empty .add {
     line-height: 1.6em;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .full .halloeditor,
     .full input,
@@ -717,5 +717,5 @@ footer .preview button {
     border-color: #666;
 }
-@media screen and (max-width: 50em) {
+@media screen and (max-width: 56.1875em) {
     footer .preview .button,
     footer .preview button {
@@ -753,5 +753,5 @@ footer .preview .dropdown.open > .button + .dropdown-toggle {
     background-color: #4d4d4d;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .object fieldset {
         display: block;
git diff -U2 --no-index --no-color a/wagtail-master/admin/static/wagtailadmin/css/panels/draftail.css b/wagtail/admin/static/wagtailadmin/css/panels/draftail.css
index d4ae7dfc4..09701f29e 100644
--- a/wagtail-master/admin/static/wagtailadmin/css/panels/draftail.css
+++ b/wagtail/admin/static/wagtailadmin/css/panels/draftail.css
@@ -534,5 +534,5 @@ button[data-draftail-balloon] {
     padding-right: 20px;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .full .Draftail-Editor .public-DraftEditor-content,
     .full .Draftail-Editor .public-DraftEditorPlaceholder-root {
@@ -545,5 +545,5 @@ button[data-draftail-balloon] {
     margin-right: 20px;
 }
-@media only screen and (min-width: 50em) {
+@media screen and (min-width: 50em) {
     .full .Draftail-Toolbar {
         margin-left: 50px;
@jjanssen

This comment has been minimized.

Member

jjanssen commented Nov 4, 2018

Let me take a look at those last few changes. It shouldn't be on your plate just because I didn't do a proper double-check on other files.

Regarding your question. I don't think we even should have the media-breakpoint-down in the most ideal situation. My intention with this 1st iteration was to eventually refactor into a solution where mobile is targeted first and work our media-breakpoint-up from there. But that would be a hard pill too swallow just from a single pull request.

@thibaudcolas thibaudcolas added this to the 2.4 milestone Nov 4, 2018

thibaudcolas added some commits Nov 4, 2018

@thibaudcolas

馃憣 all good now! I only tested this very briefly in Chrome only, as the built CSS is mostly identical (removed off-by-1px overlaps, only keyword, and the removed footer styles)

@thibaudcolas thibaudcolas merged commit 487f1f4 into wagtail:master Nov 4, 2018

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
ci/circleci: backend Your tests passed on CircleCI!
Details
ci/circleci: frontend Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 1efad23...16fb416
Details
codecov/project 98.58% (+4.92%) compared to 1efad23
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment