eslint icon indicating copy to clipboard operation
eslint copied to clipboard

chore: fix off-by-one `min-width: 1023px` media queries

Open mdjermanovic opened this issue 3 years ago • 4 comments

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update [ ] Bug fix (template) [ ] New rule (template) [ ] Changes an existing rule (template) [ ] Add autofix to a rule [ ] Add a CLI option [ ] Add something to the core [x] Other, please explain:

Updates css for the new docs site.

We have min-width: 1023px in several media queries. This seems like an off-by-one error as it matches >= 1023px which was probably not the intention.

What changes did you make? (Give an overview)

Changed min-width: 1023px to min-width: 1024px in media queries.

Is there anything you'd like reviewers to focus on?

max-width: 800px in multiple places also looks suspicious, but I wasn't sure about that one so I left it out from this PR.

mdjermanovic avatar Jun 08 '22 12:06 mdjermanovic

Deploy Preview for docs-eslint ready!

Name Link
Latest commit e4ef3962a0026408c84e1401e9d70714063f078e
Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/62a096dd09324d0008f8e3e0
Deploy Preview https://deploy-preview-15974--docs-eslint.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Jun 08 '22 12:06 netlify[bot]

@SaraSoueidan can you comment on this? Is this intentional?

nzakas avatar Jun 09 '22 18:06 nzakas

@nzakas It was intentional, yes.

It's not a new practice. It ensures the “desktop” media queries aren't triggered on a 1024p-wide viewport, which has typically been a tablet (such as iPad) size. This practice has been carved in my brain for years that it comes naturally now.

If @mdjermanovic has tested the site on medium-size displays and found no issues, then this PR LGTM.

SaraSoueidan avatar Jun 28 '22 02:06 SaraSoueidan

It ensures the “desktop” media queries aren't triggered on a 1024p-wide viewport, which has typically been a tablet (such as iPad) size.

I'm confused now. If we don't want these styles to apply on 1024px, then shouldn't this actually be min-width: 1025px?

Before & after this change are same on 1024px:

1024px before & after this change

image

But different on 1023px, because both min-width: 1023px and max-width: 1023px styles that we have apply when the width is exactly 1023px:

1023px before this change

image

1023px after this change

image

mdjermanovic avatar Jun 28 '22 16:06 mdjermanovic

I followed up with Sara (she’s been extremely busy) and she said to go ahead and merge this.

nzakas avatar Aug 26 '22 16:08 nzakas