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

Change top nav from accordion to two rows on mobile #1690

Merged
merged 10 commits into from
Dec 27, 2021
Merged

Change top nav from accordion to two rows on mobile #1690

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c3f35c5
Redesign top nav to two rows
jonahtanjz Dec 7, 2021
8503548
Update navbar snapshot test
jonahtanjz Dec 7, 2021
4fa4314
Run lintfix
jonahtanjz Dec 7, 2021
92b2a2e
Add comments to css
jonahtanjz Dec 7, 2021
386fc8f
Improve mobile navbar and dropdown
jonahtanjz Dec 11, 2021
506cb14
Update snapshot test
jonahtanjz Dec 11, 2021
d18a902
Update after review
jonahtanjz Dec 13, 2021
fe1e578
Update after review
jonahtanjz Dec 18, 2021
270a51b
Update navbar test
jonahtanjz Dec 18, 2021
b3dd82f
Improve default navbar implementation
jonahtanjz Dec 26, 2021
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
41 changes: 39 additions & 2 deletions packages/vue-components/src/Dropdown.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import Submenu from './Submenu.vue';
import { toBoolean } from './utils/utils';
import $ from './utils/NodeList';
import preventOverflowOnMobile from './utils/dropdown';

export default {
components: {
Expand Down Expand Up @@ -84,6 +85,9 @@ export default {
hasParentDropdown: {
default: undefined,
},
isParentNavbar: {
default: false,
},
},
data() {
return {
Expand Down Expand Up @@ -131,11 +135,24 @@ export default {
},
hideDropdownMenu() {
this.show = false;
$(this.$refs.dropdown).findChildren('ul').each(ul => ul.classList.toggle('show', false));
$(this.$refs.dropdown).findChildren('ul').each((ul) => {
ul.classList.toggle('show', false);

if (window.innerWidth < 768 && this.isParentNavbar) {
ul.style.removeProperty('left');
}
});
},
showDropdownMenu() {
this.show = true;
$(this.$refs.dropdown).findChildren('ul').each(ul => ul.classList.toggle('show', true));
$(this.$refs.dropdown).findChildren('ul').each((ul) => {
ul.classList.toggle('show', true);

// check if the dropdown is part of the sliding menu on mobile
if (window.innerWidth < 768 && this.isParentNavbar) {
preventOverflowOnMobile(ul);
}
});
},
},
mounted() {
Expand Down Expand Up @@ -169,6 +186,24 @@ export default {
</script>

<style scoped>
@media (max-width: 767px) {
.navbar-default .dropdown {
position: static;
}

.navbar-default .dropdown-menu {
position: absolute;
max-width: 100%;
max-height: 75vh;
overflow-y: auto;
overscroll-behavior: contain;
}

.navbar-default .dropdown-menu-right {
right: auto;
}
}

.secret {
position: absolute;
clip: rect(0 0 0 0);
Expand All @@ -187,6 +222,8 @@ export default {

.dropdown-toggle {
cursor: pointer;
display: block;
width: max-content;
}

.navbar .dropdown-toggle {
Expand Down
156 changes: 121 additions & 35 deletions packages/vue-components/src/Navbar.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,18 @@
<div>
<nav ref="navbar" :class="['navbar', 'navbar-expand-md', themeOptions, addClass, fixedOptions]">
<div class="container-fluid">
<div class="navbar-brand">
<div class="navbar-left">
<slot name="brand"></slot>
</div>
<button
v-if="!slots.collapse"
class="navbar-toggler"
type="button"
aria-expanded="false"
aria-label="Toggle navigation"
@click="toggleCollapse"
>
<span class="navbar-toggler-icon"></span>
<slot name="collapse"></slot>
</button>

<div :class="['navbar-collapse',{collapse:collapsed}]">
<div ref="navbarDefault" class="navbar-default">
<ul class="navbar-nav mr-auto mt-2 mt-lg-0">
<slot></slot>
</ul>
<ul v-if="slots.right" class="navbar-nav navbar-right">
<slot name="right"></slot>
</ul>
</div>

<ul v-if="slots.right" class="navbar-nav navbar-right">
<slot name="right"></slot>
</ul>
</div>
</nav>
<div
Expand Down Expand Up @@ -73,12 +62,12 @@ export default {
provide() {
return {
toggleLowerNavbar: this.toggleLowerNavbar,
isParentNavbar: true,
};
},
data() {
return {
id: 'bs-example-navbar-collapse-1',
collapsed: true,
styles: {},
isLowerNavbarShowing: false,
};
Expand Down Expand Up @@ -111,10 +100,6 @@ export default {
},
},
methods: {
toggleCollapse(e) {
if (e) { e.preventDefault(); }
this.collapsed = !this.collapsed;
},
// Splits a normalised URL into its parts,
// e.g http://site.org/foo/bar/index.html -> ['foo','bar','index.html']
splitUrl(url) {
Expand Down Expand Up @@ -167,7 +152,8 @@ export default {
},
highlightLink(url) {
const defHlMode = this.defaultHighlightOn;
const navLis = Array.from(this.$el.querySelector('.navbar-nav').children);
const navLis = [];
this.$el.querySelectorAll('.navbar-nav').forEach(nav => navLis.push(...Array.from(nav.children)));
// attempt an exact match first
for (let i = 0; i < navLis.length; i += 1) {
const li = navLis[i];
Expand Down Expand Up @@ -254,35 +240,128 @@ export default {
if (!content.contains(e.target)) content.classList.remove('open');
});
});
$(this.$el).on('click', 'li:not(.dropdown)>a', (e) => {
if (e.target.classList.contains('submenu-toggle')) { return; }
setTimeout(() => { this.collapsed = true; }, 200);
}).onBlur((e) => {
if (!this.$el.contains(e.target)) { this.collapsed = true; }
});
if (this.slots.collapse) $('[data-toggle="collapse"]', this.$el).on('click', e => this.toggleCollapse(e));

// highlight current nav link
this.highlightLink(window.location.href);

// scroll default navbar horizontally to current link if it is beyond the current scroll
const currentNavlink = $(this.$refs.navbarDefault).find('.current')[0];
if (currentNavlink && window.innerWidth < 768
&& currentNavlink.offsetLeft + currentNavlink.offsetWidth > window.innerWidth) {
this.$refs.navbarDefault.scrollLeft = currentNavlink.offsetLeft + currentNavlink.offsetWidth
- window.innerWidth;
}

this.toggleLowerNavbar();
$(window).on('resize', this.toggleLowerNavbar);

// scroll default navbar horizontally when mousewheel is scrolled
$(this.$refs.navbarDefault).on('wheel', (e) => {
const isDropdown = (nodes) => {
for (let i = 0; i < nodes.length; i += 1) {
if (nodes[i].classList && nodes[i].classList.contains('dropdown-menu')) {
return true;
}
}
return false;
};

// prevent horizontal scrolling if the scroll is on dropdown menu
if (window.innerWidth < 768 && !isDropdown(e.path)) {
e.preventDefault();
this.$refs.navbarDefault.scrollLeft += e.deltaY;
}
});
},
beforeDestroy() {
$('.dropdown', this.$el).off('click').offBlur();
if (this.slots.collapse) $('[data-toggle="collapse"]', this.$el).off('click');
$(window).off('resize', this.toggleLowerNavbar);
$(this.$refs.navbarDefault).off('wheel');
},
};
</script>

<style scoped>
@media (max-width: 767px) {
.navbar-collapse {
max-height: 80vh !important;
overflow-x: hidden !important;
overflow-y: scroll !important;
.navbar {
padding-left: 0;
padding-right: 0;
padding-bottom: 0;
}

.navbar-left {
max-width: 50%;
order: 1;
padding-left: 1rem;
}

.navbar-left * {
white-space: normal;
}

.navbar-right {
order: 1;
max-width: 50%;
Copy link
Contributor

@ang-zeyu ang-zeyu Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leftbrand

(also need to take note 50% does not account for the margin-right set by navbar-brand in bootstrap)

Discovered a few pre-existing issues with the navbar's flex parameters while reviewing as well:

cropped

(and a few others, try resizing with a long list of navbar contents)

Would be good if you are able to get a generalised fix / implementation out here, if there's one 😃 (on second thought let's keep in mind and fix separately; found a few more issues that seems to be out of scope here)
But if not we can always keep in mind to to fix soon in the next PR or so, and just focus on the mobile version here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also need to take note 50% does not account for the margin-right set by navbar-brand in bootstrap

Thanks for the catch! I've factored the margin into the max-width now and also set white-space: normal so that items within navbar-brand can wrap to next line when it exceeds 50%.

on second thought let's keep in mind and fix separately; found a few more issues that seems to be out of scope here.
But if not we can always keep in mind to to fix soon in the next PR or so, and just focus on the mobile version here.

Yup sure. I can open an issue after this PR to keep track of it :)

padding: 0 16px;
}

.navbar-default {
display: block;
margin-top: 0.3125rem;
width: 100%;
order: 2;
overflow-x: scroll;

/* Hide overflow scroll bar */
-ms-overflow-style: none; /* IE and Edge */
scrollbar-width: none; /* Firefox */
}

/* Hide overflow scroll bar for Chrome and Safari */
.navbar-default::-webkit-scrollbar {
display: none;
}

.navbar-default ul {
flex-direction: row;
margin-top: 0 !important;
width: 100%;
}

.navbar-default > ul > * {
background: rgba(0, 0, 0, 0.2);
padding: 0.3125rem 0.625rem;
flex-grow: 1;
}

.navbar-light .navbar-default > ul > * {
background: rgba(0, 0, 0, 0.05);
}

.navbar-default > ul > .current {
background: transparent;
}

.navbar-default a,
>>> .dropdown-toggle {
margin: 0 auto;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems to push the last few item(s) after the dropdown to the right (if there's extra space)

tried removing it and looks good; what's it for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh this is for center aligning the text in the default navbar in some screen sizes when the width of each link is slightly larger.

With margin: 0 auto:
image

Without margin: 0 auto:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm in this case would all the items be better flushed to the left (or center or right), not stretched out?

It makes the location and blank space of elements slightly more predictable with respect to how much content they contain (and more consistent to the design when overflow occurs).

e.g. huge space between dropdown and "home" leads to inconsistent "eye travel distance", since the content here can be of varying lengths.
spacing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh ok got it. I've flushed all items to the left and it looks alot better.

width: max-content;
}

>>> .dropdown {
display: flex;
align-items: center;
}
}

.navbar-left {
display: inline-block;
font-size: 1.25rem;
line-height: inherit;
padding-right: 1rem;
padding-top: 0.3125rem;
padding-bottom: 0.3125rem;
white-space: nowrap;
}

.navbar-fixed {
Expand All @@ -291,6 +370,13 @@ export default {
z-index: 1000;
}

.navbar-default {
display: flex;
flex-basis: auto;
flex-grow: 1;
align-items: center;
}

>>> .dropdown-current {
color: #fff !important;
background: #007bff;
Expand Down
6 changes: 5 additions & 1 deletion packages/vue-components/src/Searchbar.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,13 @@ export default {

@media screen and (max-width: 768px) {
.search-dropdown-menu {
min-width: auto;
min-width: 90vw;
max-height: 30em;
overflow-y: scroll;
}

.dropdown-menu.search-dropdown-menu {
position: absolute;
}
}
</style>
13 changes: 13 additions & 0 deletions packages/vue-components/src/Submenu.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import { toBoolean } from './utils/utils';
import $ from './utils/NodeList';
import positionSubmenu from './utils/submenu';
import preventOverflowOnMobile from './utils/dropdown';

export default {
props: {
Expand All @@ -44,6 +45,11 @@ export default {
dropleft: false,
};
},
inject: {
isParentNavbar: {
default: false,
},
},
computed: {
disabledBool() {
return toBoolean(this.disabled);
Expand All @@ -59,6 +65,13 @@ export default {
this.show = true;
$(this.$refs.submenu).findChildren('ul').each((ul) => {
ul.classList.toggle('show', true);

// check if submenu is part of the navbar sliding menu on mobile
if (window.innerWidth < 768 && this.isParentNavbar) {
preventOverflowOnMobile(ul);
return;
}

if (positionSubmenu.isRightAlign(ul)) {
this.alignMenuRight();
} else {
Expand Down
Loading