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

docs: align footer-tablet view #15918

Closed

Conversation

DeepshikaS
Copy link
Contributor

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
  • Other, please explain:

What changes did you make? (Give an overview)

Changed the footer alignment in docs for tablet view.

Before :

image

After :

image

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon documentation Relates to ESLint's documentation labels May 24, 2022
@netlify
Copy link

netlify bot commented May 24, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 240362a
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/628d940fd833960008cf3bec
😎 Deploy Preview https://deploy-preview-15918--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.

@@ -7,6 +7,7 @@

@media all and (max-width: 800px) {
padding: 1.5rem 0 4rem;
align-items:center ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
align-items:center ;
align-items: center;

@snitin315 snitin315 added accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels May 25, 2022
Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Let's keep it open for other team members to review.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

Noticed one oddity at exactly 800px after I submitted my initial review. Leaving this as a comment rather than request changes because I don't think it should block merging if changing the breakpoint feels out of scope for this change.

@@ -7,6 +7,7 @@

@media all and (max-width: 800px) {
Copy link
Member

Choose a reason for hiding this comment

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

Try setting your view width to exactly 800px. It'll give a combination of the tablet and desktop views:

Screen Shot 2022-06-01 at 1 05 08 AM

That's because there's a min-width: 800px media query that makes .docs-content a grid.

Currently, the footer is centered at widths <= 800px, and the page is a grid at >= 800px, so there's a bit of overlap. If we make this a max-width of 799px, that overlap goes away:

Suggested change
@media all and (max-width: 800px) {
@media all and (max-width: 799px) {

@@ -7,6 +7,7 @@

@media all and (max-width: 800px) {
padding: 1.5rem 0 4rem;
align-items: center;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is included in #15944

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay!

@DeepshikaS
Copy link
Contributor Author

Closing this PR in favor of #15944

@DeepshikaS DeepshikaS closed this Jun 5, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 3, 2022
@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 Dec 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants