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

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

Merged
merged 1 commit into from Aug 26, 2022

Conversation

mdjermanovic
Copy link
Member

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 mdjermanovic added documentation Relates to ESLint's documentation chore This change is not user-facing labels Jun 8, 2022
@netlify
Copy link

netlify bot commented Jun 8, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit e4ef396
🔍 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.

@nzakas
Copy link
Member

nzakas commented Jun 9, 2022

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

@SaraSoueidan
Copy link

@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.

@mdjermanovic
Copy link
Member Author

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

@nzakas
Copy link
Member

nzakas commented Aug 26, 2022

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

@nzakas nzakas merged commit deaf69f into main Aug 26, 2022
@nzakas nzakas deleted the mediaqueries-offbyone branch August 26, 2022 16:51
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Sep 1, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.22.0` -> `8.23.0`](https://renovatebot.com/diffs/npm/eslint/8.22.0/8.23.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.23.0`](https://github.com/eslint/eslint/releases/tag/v8.23.0)

[Compare Source](eslint/eslint@v8.22.0...v8.23.0)

#### Features

-   [`3e5839e`](eslint/eslint@3e5839e) feat: Enable eslint.config.js lookup from CLI ([#&#8203;16235](eslint/eslint#16235)) (Nicholas C. Zakas)
-   [`30b1a2d`](eslint/eslint@30b1a2d) feat: add `allowEmptyCase` option to no-fallthrough rule ([#&#8203;15887](eslint/eslint#15887)) (Amaresh  S M)
-   [`43f03aa`](eslint/eslint@43f03aa) feat: no-warning-comments support comments with decoration ([#&#8203;16120](eslint/eslint#16120)) (Lachlan Hunt)

#### Documentation

-   [`b1918da`](eslint/eslint@b1918da) docs: package.json conventions ([#&#8203;16206](eslint/eslint#16206)) (Patrick McElhaney)
-   [`0e03c33`](eslint/eslint@0e03c33) docs: remove word immediately ([#&#8203;16217](eslint/eslint#16217)) (Strek)
-   [`c6790db`](eslint/eslint@c6790db) docs: add anchor link for "migrating from jscs" ([#&#8203;16207](eslint/eslint#16207)) (Percy Ma)
-   [`7137344`](eslint/eslint@7137344) docs: auto-generation edit link ([#&#8203;16213](eslint/eslint#16213)) (Percy Ma)

#### Chores

-   [`2e004ab`](eslint/eslint@2e004ab) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).3.1 ([#&#8203;16249](eslint/eslint#16249)) (Milos Djermanovic)
-   [`d35fbbe`](eslint/eslint@d35fbbe) chore: Upgrade to espree@9.4.0 ([#&#8203;16243](eslint/eslint#16243)) (Milos Djermanovic)
-   [`ed26229`](eslint/eslint@ed26229) test: add no-extra-parens tests with rest properties ([#&#8203;16236](eslint/eslint#16236)) (Milos Djermanovic)
-   [`deaf69f`](eslint/eslint@deaf69f) chore: fix off-by-one `min-width: 1023px` media queries ([#&#8203;15974](eslint/eslint#15974)) (Milos Djermanovic)
-   [`63dec9f`](eslint/eslint@63dec9f) refactor: simplify `parseListConfig` ([#&#8203;16241](eslint/eslint#16241)) (Milos Djermanovic)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xNzcuMSIsInVwZGF0ZWRJblZlciI6IjMyLjE4MS4wIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1527
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 23, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion chore This change is not user-facing documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants