-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fixed the home page Footer mobile responsive issue #5964
Conversation
Welcome @ShravaniAK! It looks like this is your first PR to knative/docs 🎉 |
✅ Deploy Preview for knative ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few code quality comments then this is good to merge, awesome work @ShravaniAK !
@@ -352,7 +352,7 @@ h2.secondary-headline, h3.trusted-by { | |||
justify-content: center; | |||
color: black; | |||
display: flex; | |||
background-color: white; | |||
background-color: #f3f1f2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you reference one of the css custom properties here instead of a hard coded css value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah sure !
} | ||
.md-footer-copyright{ | ||
margin-right: 18px; | ||
} | ||
@media (max-width: 1024px){ | ||
.md-footer-copyright{ | ||
margin: auto; | ||
text-align: center; | ||
} | ||
.md-social{ | ||
margin: auto; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you add spacing between the classes here and fix the indentation so that this is easier to read?
margin: auto; | ||
} | ||
} | ||
@media (max-width: 861px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between this PR and the other responsiveness one it seems like we have used three screen width queries with different widths so far...
Would it be possible to standardize to maybe 2 widths to check? One for medium and one for small screen sizes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707, ShravaniAK The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
currently, the Footer is not responsive,
I have made the Footer responsive on all screens,