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

Fixed website overflow issue in mobile and height of navbar #493

Merged
merged 1 commit into from Jun 14, 2020

Conversation

shubhamV123
Copy link
Contributor

Fixed height of navbar on mobile and flickering scrolling of navbar in mobile. Remove redundant style from .grid-toc class

}
else{
document.body.style.position = 'fixed';
}
Copy link
Collaborator

@drwpow drwpow Jun 14, 2020

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 the position is related directly to the .hidden-mobile class logic, why don’t we move that position: 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.

Copy link
Contributor Author

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 a class on grid-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 with hidden-mobile so I fixed body height whenever the mobile sidebar is open and removed when it's closed

Copy link
Collaborator

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.

@drwpow drwpow merged commit ab33a43 into FredKSchott:master Jun 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants