Change top nav from accordion to two rows on mobile #1690
Conversation
|
Nice! Reviewing the design first; I think it looks good overall, just 2 parts that need to be fixed:
|
|
Hi @ang-zeyu thanks for the review. I've made the changes accordingly.
I believe to scroll horizontally with the mouse wheel can hold down
Yup agree that it will look better. I've updated the design so that it is now absolute positioned and also added scrolling if the dropdown overflows the bottom of the screen.
|
There was a problem hiding this comment.
I believe to scroll horizontally with the mouse wheel can hold down SHIFT and scroll but this doesn't really seem intuitive 😅 I've bind the mouse wheel to the horizontal scroll on the mobile navbar so this should resolve the issue.
forgot about that 😅, but yes, agree it isn't as intuitive
I only glossed over the positioning / overflow implementation which seems quite complex, it seems to do the job quite well though!
Mostly nits on the styling implementation otherwise:
| $(this.$refs.dropdown).findChildren('ul').each((ul) => { | ||
| ul.classList.toggle('show', false); | ||
|
|
||
| if (window.innerWidth < 768 && this.$refs.dropdown.closest('div.navbar-default') !== null) { |
There was a problem hiding this comment.
think we could use provide / inject here again to avoid coupling to .navbar-default
There was a problem hiding this comment.
Yup this seems alot better. Thanks!
| $(window).off('resize', this.toggleLowerNavbar); | ||
| $(this.$el.querySelector('.navbar-default')).off('wheel'); |
There was a problem hiding this comment.
does using the ref you created work? $refs.navbarDefault
(the one in submenu as well)
There was a problem hiding this comment.
Yup using the ref works. I did this part before creating the ref and forgot to update it. Thanks for the catch
| } | ||
|
|
||
| .navbar-right { | ||
| position: absolute; |
There was a problem hiding this comment.
try the flex order property; with the right combination you should be able to remove the reliance on absolute positioning (not too good in this case as its sizing should be taken into account in the document flow, e.g. if navbar-brand is empty this overflows the container)
There was a problem hiding this comment.
Thanks for the suggestion 👍 This works and looks alot better!
|
|
||
| .navbar-default a, | ||
| >>> .dropdown-toggle { | ||
| margin: 0 auto; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
Without margin: 0 auto:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ahh ok got it. I've flushed all items to the left and it looks alot better.
| .navbar-default > ul > div { | ||
| padding: 0.3125rem 0.625rem; | ||
| flex: 1; | ||
| background: rgba(0, 0, 0, 0.3); |
There was a problem hiding this comment.
its background should also change to the main colour of the navbar.
ok, I see the point now, I think this works best in that case.
The alternative I had in mind was to move the said color to .navbar-default, then forward the navbar main colour to .current specifically. This means forwarding .navbar-light/dark/primary's colours to the .current element manually.
One slight issue with 0.3 specifically though - you'll likely need to try out / set a different value for .navbar-light, 0.3 seems a little dark for it.
|
|
||
| .navbar-right { | ||
| order: 1; | ||
| max-width: 50%; |
There was a problem hiding this comment.
(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:
(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.
There was a problem hiding this comment.
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 :)
|
|
||
| .navbar-default a, | ||
| >>> .dropdown-toggle { | ||
| margin: 0 auto; |
There was a problem hiding this comment.
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.
| @@ -318,8 +332,10 @@ export default { | |||
|
|
|||
| .navbar-default > ul > * { | |||
| padding: 0.3125rem 0.625rem; | |||
| flex: 1; | |||
| background: rgba(0, 0, 0, 0.3); | |||
There was a problem hiding this comment.
👀
ok, I see the point now, I think this works best in that case.
I actually liked the old approach with setting backgroud colour on the individual ul>*'s #1690 (comment). (just need a separate .navbarlight .navbar-default > ul > * { background: ... } of sorts)
I think dynamic style detection (--mobile-link-highlight-color) is something we should avoid when possible as it may introduce more noticeable / distracting visual changes after the page has been presented already. (try seeing the netlify on a slow enough mobile device, you'll get to see the background colour change)
There was a problem hiding this comment.
Alright I've changed back to the previous implementation. Initially when I flushed all nav items to the left #1690 (comment), it left some spaces at the end on some screen sizes so I decided to apply the colour to the entire background instead.
I tried playing around with the flex properties and decided to go with flex-grow: 1 so that the nav items fill the bar completely and also ensure that the spacing between each item is "equal". I think this allows for setting the background of each individual nav link as well as addresses the spacing issue.
|
one more minor suggestion: should we scroll the scrollbar horizontally toward the "current" element if its hidden beyond the scrollbar? It might be somewhat distracting ux wise if it runs too slowly though, you could also try hoisting the scrolling logic into the |
Missed the search dropdown |
I tried inserting the scrolling script right after the highlight link function and it seems fine on the preview deployment. The scroll should happen right after the link highlighting with minimal delay.
Thanks for the catch! I've also increased the min-width of the search bar on mobile so that it has sufficient space to display the content. Earlier some contents of the search were too compressed. |
What is the purpose of this pull request?
Overview of changes:
Resolves #980
Remove current accordion for top navigation on mobile and shift it to create a second row sliding menu.
slot="right"item will now be visible and positioned on the right side instead of being in an accordion.Before:
After:
Anything you'd like to highlight / discuss:
NIL
Testing instructions:
Preview markbind site with updated top nav design: https://deploy-preview-1690--markbind-master.netlify.app/index.html
Proposed commit message: (wrap lines at 72 characters)
Change top nav from accordion to two rows on mobile
The default slot and items in the right slot are currently
hidden in an accordion on mobile.
Let's shift the default slot items to a new row so that they are
more accessible and right slot items can now be placed on the
right side of the top nav.
Checklist: ☑️