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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Jun 14, 2020
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions www/.eleventy/layouts/base.njk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,16 +492,12 @@ body {
}
@media (max-width: 740px) {
.grid-toc {
position: fixed;
top: 44px;
left: 0;
right: 0;
bottom: 0;
padding: 0;
width: 100%;
overflow: auto;
z-index: 100;
}
}
.grid-toc-buttons {
position: fixed;
top: 0;
Expand All @@ -515,6 +511,12 @@ body {
padding-top: 44px;
margin-left: 0;
}
.toc ol{
margin-bottom:0;
}
.toc > ol > li{
padding-bottom:3rem;
}
.hidden-mobile {display: none; }
.version-tag {display: none; }
#toc-logo { display: none;}
Expand Down Expand Up @@ -646,10 +648,24 @@ body {

document.getElementById('toc-drawer-button').addEventListener('click', (e) => {
e.preventDefault();
gridTocEl.classList.toggle('hidden-mobile');
/*If hidden-mobile class is enabled that means we are on desktop do overflow normal but we
if we are at mobile fixed body position, so that its not scrollable(which currently causing bug) and navbar handling its
owns scroll. Case to consider there are chance use can open navbar using toggle button and user when click on any link
body postion should be unset
*/
const ishiddenMobileClassEnabled = gridTocEl.classList.toggle('hidden-mobile');
if(ishiddenMobileClassEnabled){
document.body.style.position = "";
}
else{
document.body.style.position = 'fixed';
}
Copy link
Collaborator

@drwpow drwpow Jun 14, 2020 •
edited
Loading

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.



});
gridTocEl.addEventListener('click', (e) => {
gridTocEl.classList.add('hidden-mobile');
document.body.style.position = '';
});
/* May not be needed:
window.addEventListener('DOMContentLoaded', (event) => {
Expand Down