- Notifications
You must be signed in to change notification settings - Fork916
Fixed website overflow issue in mobile and height of navbar#493
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
height of navbar
} | ||
else{ | ||
document.body.style.position = 'fixed'; | ||
} |
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.
Thanks so much for submitting the PR! One suggestion: since theposition
is related directly to the.hidden-mobile
class logic, why don’t we move thatposition: fixed
to the CSS above? Something like:
.grid-toc { position: fixed;}.grid-toc.hidden-mobile { position: static; } /* this will always take priority with 2 CSS classes */
That way we handel less styling logic with the JS. And if we need to make other changes to the CSS we don’t forget about this down here and end up fighting with it.
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.
Hey@drwpow . Thanks for your suggestion. Initially, my thought is also the same, we don't need to add CSS logic in js. Class.hidden-mobile is a factor but apart from this body of background is also a factor. So what happens is basically we open sidebar. We didn't do anything with the grid body and it does have a lot of height so the sidebar has a background overflow and its overflow. So it has a very buggy overflow. You can see two overflows going on. What you suggested is works for the sidebar not for its background body. So initially my thought is to add aclass ongrid-body and fixed it when sidebar opens and remove it when it's closed. But it has the same logic as a hidden-mobile class so I need to add another event listener and repeat the logic for the toggle. So I came to this solution as we are already doing the same thing withhidden-mobile so I fixed body height whenever the mobile sidebar is open and removed when it's closed
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.
Ah I see what you mean now! Sorry—I was mistaken. I tested this locally and this seems to fix the issue! 🎉
I’ll merge this and we’ll deploy this this week. Thanks again.
Fixed height of navbar on mobile and flickering scrolling of navbar in mobile. Remove redundant style from .grid-toc class