Skip to content

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

Merged
drwpow merged 1 commit intoFredKSchott:masterfrom
shubhamV123:master
Jun 14, 2020
Merged

Fixed website overflow issue in mobile and height of navbar#493
drwpow merged 1 commit intoFredKSchott:masterfrom
shubhamV123:master

Conversation

@shubhamV123
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants