Make navigation menus accessible on smaller screens #1445

jonahtanjz · 2021-01-14T18:55:44Z

What is the purpose of this pull request?

Overview of changes:
Fixes #980, fixes #591.

Currently, siteNav and pageNav are hidden when screen size is small (< 993px). This PR adds an additional navigation bar below topNav to toggle the siteNav and pageNav menu when screen is small (similar to Docusaurus design).

This is done by adding additional HTML code directly in index.js of Page, similar to how scroll to top button is implemented. Additional Javascript is also added to help with the toggling of the buttons. To check if a page contains siteNav or pageNav, a <script> tag is added to siteNavProcessor.js and index.js of Page to call the custom js function to let it know if they are present respectively on the page.

Anything you'd like to highlight / discuss:
Most of the functions are binded to window and the function calls are done using <script>. Is this method of implementation recommended? Is there a better way to call these functions when building the siteNav or pageNav?

Testing instructions:

Create a site with siteNav and pageNav
Decrease screen size to less than 993px
Navigation menu should appear below topNav

Proposed commit message: (wrap lines at 72 characters)
Implement mobile site and page navigation menus

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No blatantly unrelated changes
Pinged someone for a review!

ang-zeyu

Thanks! @jonahtanjz

Regarding the usage (from author pov), I think we could try to encapsulate the implementation in the already existing Navbar.vue, and create some reusable components. Something along the lines of this that dosen't make this a fixed, mandatory placement:

// author writing a navbar
<navbar>
  ...
  <div slot="lower-navbar"> // a new slot for the "secondary navbar" you have here
    <page-nav-button> // new component the user may use to link this up
    <site-nav-button> // similarly
  </div>
</navbar>

that way, we don't restrict the author to a single layout (secondary navbar), they may even omit the secondary navbar entirely, or place the buttons in the "main (top most) navbar" if they wish.

we could also provide customisation options for page-nav-button / etc. in the future.

With the comments below:

Most of the functions are binded to window and the function calls are done using <script>. Is this method of implementation recommended? Is there a better way to call these functions when building the siteNav or pageNav?

So I guess, the answer to this lies in the (author) usage design. Changing it should result in a more sealed off implementation as well.

There are probably better ways of doing this besides the two suggestions here, but this is what I can propose right now 🤔🤔🤔

Would love to hear your suggestions @jonahtanjz @damithc @acjh

ang-zeyu · 2021-01-15T05:08:10Z

packages/core-web/src/navMenuBar.js

+
+function toggleSiteNav() {
+  if (!navMenuStates.siteNavOpened) {
+    document.getElementById('site-nav').classList.add('nav-menu-open');


an element with id site-nav may not exist at all, its only in the layout files to serve as organizers / identifiers / some default styling which the author may discard

As the site-nav, page-nav is "component-fied", although unlikely, we may have multiple in the page, which makes assigning identifiers to them ineffective (if we try to move site-nav / ~~page-nav~~ onto <site-nav> / ~~{{ pageNav }}~~) (forgot pageNavs were already limited to one 🙈, that would have to be changed if we go with this)

I'm wondering if we should create a generic overlay component instead (in fact bootstrap-vue has a b-sidebar component we could easily use)

// author writing a navbar <navbar> ... <div slot="lower-navbar"> // a new slot for the "secondary navbar" you have here <overlay icon="..."> <site-nav> ... </site-nav> </overlay> <overlay icon="..."> {{ pageNav }} </overlay> </div> </navbar>

This way, the author is able to show different content for the mobile navbar overlays. Content repetition can be easily resolved with <include>s

The downsides are

still more things to write for the author

I imagine the overlay wouldn't exactly be like docusaurus', since its a generic component. (would cover the entire screen, instead of from the navbar downward)

this approach also likely results in zero changes needed to the backend code - slightly better decoupling

ang-zeyu · 2021-01-15T05:08:20Z

packages/core-web/src/navMenuBar.js

+}
+
+function closePageNav() {
+  document.getElementById('page-nav').classList.remove('nav-menu-open');


ang-zeyu · 2021-01-15T05:27:33Z

packages/core-web/src/styles/nav-menu-bar.css

@@ -0,0 +1,65 @@
+@media (min-width: 993px) {


nice work styling this. we can very likely reuse the work here still

ang-zeyu · 2021-01-15T09:13:21Z

packages/core/src/Page/index.js

@@ -347,7 +347,7 @@ class Page {
    const headingStack = [];
    Object.keys(this.navigableHeadings).forEach((key) => {
      const currentHeadingLevel = this.navigableHeadings[key].level;
-      const currentHeadingHTML = `<a class="nav-link py-1" href="#${key}">`
+      const currentHeadingHTML = `<a onclick="closePageNav();" class="nav-link py-1" href="#${key}">`


we may catch the bubbled event instead (also checking e.target and other parameters to make it as restrictive as possible) to hide the overlay. This way it stays decoupled

jonahtanjz · 2021-01-18T07:18:38Z

Thanks @ang-zeyu for the suggestions and review.

Yup I agree that making the site-nav and page-nav buttons into reusable components would allow the implementation to be more customizable and cleaner. As mentioned, doing this will require users to add in additional code to show these buttons on mobile but this should be fine if the user intent to customize it. Is it also possible if we default it to the secondary bar if it is not specified? This way, current Markbind sites will not require additional code to make these buttons appear on mobile. Not sure which is better 🤔.

I can probably start working on making the <site-nav-button> and <page-nav-button> components first if it fine to go in this direction.

ang-zeyu · 2021-01-18T08:23:38Z

Any thoughts? @damithc

Option 1:

+ prettier (overlay would extend from navbar downward only)
+ easier to use
- restrictive, overlay may only contain a pagenav (no titles, etc.), or one (the first) <site-nav>

// author writing a navbar
<navbar>
  ...
  <div slot="lower-navbar"> // a new slot for the "secondary navbar" you have here
    <page-nav-button> // new component the user may use to link this up
    <site-nav-button> // similarly
  </div>
</navbar>

Option 2:

+ generic, easy to implement
+ overlay may contain different content / designs
- more verbose for the author,
- less pretty (overlay would cover the entire page), unless we make this a <navbar> specific component (much like <quiz> / <question> pair)

// author writing a navbar
<navbar>
  ...
  <div slot="lower-navbar"> // a new slot for the "secondary navbar" you have here
    <overlay icon="...">
       <site-nav>
          ...
       </site-nav>
    </overlay>
    <overlay icon="...">
        {{ pageNav }} 
    </overlay>
  </div>
</navbar>

@jonahtanjz In either case, feel free to make the {{ pageNav }} variable an element instead as well without deprecating anything, as its unreleased.

damithc · 2021-01-18T12:43:44Z

Any thoughts? @damithc

I need some info to understand this one.
Is this going to affect how authors create navbars? Because the feature being added doesn't sound like something the author should specifically know about i.e., it should work behind the scene.
I guess it's fine to tweak the syntax but the new syntax should not reveal anything about smaller screens?

ang-zeyu · 2021-01-18T13:57:05Z

Is it also possible if we default it to the secondary bar if it is not specified?

Sorry missed this out earlier. Does this mean page / site-nav-button would not need to be placed under a slot?

I need some info to understand this one.
Is this going to affect how authors create navbars? Because the feature being added doesn't sound like something the author should specifically know about i.e., it should work behind the scene.
I guess it's fine to tweak the syntax but the new syntax should not reveal anything about smaller screens?

yup.

the main issue here is that shifting to expressive layouts means there's no longer a mandatory "site-nav section". (can't know which section of the layout file that is supposed to be) (versus before, all files in _markbind/navigation could be identified)

We certainly could identify <site-nav>, or {{ pageNav }}, but then the overlay would and can only contain these. (e.g. no "User Guide" title, images, etc. ). There's also the issue of being able to use multiple <site-nav>s, and how that would work with the mobile overlay. (which / all of the <site-nav>s should be pulled into the overlay automatically)

My opinion here is that the placement, styling of the button, etc. should not be fixed either (e.g. the author should be able to omit it entirely, or place it somewhere else, change the icon, ...). Suggestions hence:

option 1 - dosen't really deal with the main issue, but provides basic customisability (<site/page-nav-button options... />)
option 2 - deals with the main issue, but is rather verbose (<overlay options...><include src="#site-nav" /></overlay>)

We could also stick the current PR, which means nothing to do for the author at all - but it would still be a little restrictive (e.g. no "User Guide" title, images, etc. ).

(Option 3) Or minimally, we can require the author to specify some sort of identifier in any element in the layout file https://markbind-master.netlify.app/userguide/tweakingthepagestructure#layouts (e.g. id="mobile-site-nav") that tells our code what to pull into the overlay. (but then no customisability)

Not sure if there are better compromises

damithc · 2021-01-18T14:22:56Z

Related question: does this require the author to specify two site-navs, one for normal use and one for mobile? If yes, that feels like extra work an author shouldn't have to do.

jonahtanjz · 2021-01-18T15:10:57Z

Sorry missed this out earlier. Does this mean page / site-nav-button would not need to be placed under a slot?

Was thinking more along the lines of having a slot with fallback content in the navbar or something similar so that current Markbind sites will not need to do anything to be able to have the buttons on mobile.

ang-zeyu · 2021-01-19T14:27:10Z

Was thinking more along the lines of having a slot with fallback content in the navbar or something similar so that current Markbind sites will not need to do anything to be able to have the buttons on mobile.

Ahh, that sounds nice 👍

Related question: does this require the author to specify two site-navs, one for normal use and one for mobile? If yes, that feels like extra work an author shouldn't have to do.

Would require some extra things inside the existing <navbar> (not a separate one)

With @jonahtanjz 's proposal above, it is also optional (can be omitted, but also customised if needed)

damithc · 2021-01-19T14:29:05Z

Would require some extra things inside the existing <navbar> (not a separate one)

With @jonahtanjz 's proposal above, it is also optional (can be omitted, but also customised if needed)

Sounds good. 👍

ang-zeyu · 2021-01-19T14:32:10Z

The main issue still exists though (identifying what to pull into the overlay).

Suggestions:

Assume the user is only using <= 1 sitenav and <= 1 pagenav per page, then pull these in automatically if they exist.
To facilitate more expressive layouts, we can fallback to @jonahtanjz's proposal and something like option 2 (<overlay>...any MarkBind content... can be included as well...</overlay>), or something similar
Or, require the author to pull specify some identifier in the layout file of what to pull in (e.g. id="mobile-site-nav"), also combining with @jonahtanjz 's proposal but something like option 1 for customisability (<site/page-nav-button />)
❔

jonahtanjz · 2021-01-21T04:17:08Z

Assume the user is only using <= 1 sitenav and <= 1 pagenav per page, then pull these in automatically if they exist.
To facilitate more expressive layouts, we can fallback to @jonahtanjz's proposal and something like option 2 (<overlay>...any MarkBind content... can be included as well...</overlay>), or something similar

Or, require the author to pull specify some identifier in the layout file of what to pull in (e.g. id="mobile-site-nav"), also combining with @jonahtanjz 's proposal but something like option 1 for customisability (<site/page-nav-button />)

@ang-zeyu I would say option 2 is better as it is more clear and straightforward. This will allow users to specify which site-nav to use to increase flexibility but this will also mean current markbind sites will need to include this new identifier for it to work. We can also pull the default identifier, id=site-nav and id=page-nav, as a workaround for users that did not change them? Not sure how many current markbind sites have changed them. That's my thought on this.

ang-zeyu · 2021-01-21T06:43:07Z

@ang-zeyu I would say option 2 is better as it is more clear and straightforward. This will allow users to specify which site-nav to use to increase flexibility but this will also mean current markbind sites will need to include this new identifier for it to work.

👍 let's go with that then

Could also still add in the default behaviour of assuming <= 1 page/sitenav (then pulling those in at a lower priority), so the author dosen't have to do anything. (if the page dosen't contain id=site-nav nor id=page-nav)

We can also pull the default identifier, id=site-nav and id=page-nav, as a workaround for users that did not change them? Not sure how many current markbind sites have changed them. That's my thought on this.

Since its a new feature, don't worry about backward compatibility. Our template sites (markbind init) have these identifiers set up already, which should make things slightly easier as well.

So, in this order:

pull in id=site-nav / id=page-nav if they exist in the page (we can add on in another PR to change this identifier even if need be)
pull in the first <site-nav> / the only {{ pageNav }} if it exists

Sounds right? @damithc @jonahtanjz

damithc · 2021-01-21T06:56:51Z

So, in this order:

pull in id=site-nav / id=page-nav if they exist in the page (we can add on in another PR to change this identifier even if need be)

pull in the first <site-nav> / the only {{ pageNav }} if it exists

Sounds right? @damithc @jonahtanjz

Yes, no objections from me. 👍

jonahtanjz · 2021-02-02T14:50:12Z

@ang-zeyu The PR is ready for review.

I've created separate vue components for <site-nav-button /> and <page-nav-button />. For now, they can be used under a slot in the navbar so that when it is opened, the menus extend nicely below the navbar. Both components will pull in identifier first id=site/page-nav then fallback to the first or only site/page navigation respectively. Likewise, I've also included fallback content in the lower-navbar slot so that if the <site/page-nav-button /> components are not specified, they will still show up by default. Currently, both components are navbar specific as some of the logics are handled in the navbar (hiding and showing buttons on different screen sizes, as well as adjust the fixed header padding when the secondary navbar appears/disappears). Perhaps in the future if we want to increase customizability, we can extract it out of the navbar and have a custom wrapper component just for the buttons 🤔?

Also added tests and documentations :)

ang-zeyu

Thanks for the rework! : )

@ang-zeyu The PR is ready for review.

I've created separate vue components for <site-nav-button /> and <page-nav-button />. For now, they can be used under a slot in the navbar so that when it is opened, the menus extend nicely below the navbar. Both components will pull in identifier first id=site/page-nav then fallback to the first or only site/page navigation respectively. Likewise, I've also included fallback content in the lower-navbar slot so that if the <site/page-nav-button /> components are not specified, they will still show up by default.

Couple of user issues:

not scrollable (tried on mobile device + chrome resized)
the page nav button shows up when there's no page nav

Currently, both components are navbar specific as some of the logics are handled in the navbar (hiding and showing buttons on different screen sizes, as well as adjust the fixed header padding when the secondary navbar appears/disappears). Perhaps in the future if we want to increase customizability, we can extract it out of the navbar and have a custom wrapper component just for the buttons 🤔?

Also added tests and documentations :)

This sounds good, also not just for customisability - but separation of concerns

Left a few suggestions below on how to tackle the separation here (customisability can come later)

ang-zeyu · 2021-02-03T07:02:39Z

packages/vue-components/src/utils/utils.js

@@ -159,3 +161,40 @@ export function VueFixer (vue) {
  vue.mixins.unshift(mixin)
  return vue
 }
+
+export function resizeHeader(isNavMenuShowing) {


hmm not sure if we should be putting this within navbar.vue. (note a header isn't limited to just a navbar)

how about adding the window listener in index.js where the fixed header padding is originally setup?
It should keep these concerns closer.

Also, could override the selectors' properties (by cascading) instead querying every element

Also, could override the selectors' properties (by cascading) instead querying every element

Sorry I don't really get what you meant by this 😅. Do you mean inserting new CSS rules to target these classes?

Thanks for the clarification :)

hmm not sure if we should be putting this within navbar.vue. (note a header isn't limited to just a navbar)

how about adding the window listener in index.js where the fixed header padding is originally setup?
It should keep these concerns closer.

One more thing, the reason why I placed this in navbar is because a header padding resize is only necessary if the lower-navbar slot is present. In this case, is it ok to let the listener stay in navbar?

One more thing, the reason why I placed this in navbar is because a header padding resize is only necessary if the lower-navbar slot is present. In this case, is it ok to let the listener stay in navbar?

not necessarily, e.g.

<header><box dismissible>...</box></header> -> close box -> fixed-header-padding is not changed (everything appears pushed downward)

It's part of a larger generic issue (not listed anywhere) that can be dealt with separately, or in this PR since its a prerequisite (preferably a separate commit)

Ahh thanks, wasn't aware of this issue. Added a listener in index.js to resize the header if it changes. Should be fixed now :)

ang-zeyu · 2021-02-03T07:06:16Z

packages/vue-components/src/PageNavButton.vue

+      this.toggleSiteNavButton();
+    },
+    toggleSiteNavButton() {
+      const siteNavButton = document.getElementById('toggle-site-nav-button');


would :class work? Similar to what you did for nav-menu-open above. (also for the page nav variant)

Hmm I'm not sure if :class would work for this because this is querying page-nav-button from site-nav-button when it is opened. They should be considered sibling elements so not sure how to pass the data to each other unless we pass it to the parent and then back to the child. Will just using getElementById be better because the buttons id are fixed and will not be changed by the user 🤔?

ahh, my bad. I didn't notice that this was for the other component at all. 🙈

They should be considered sibling elements so not sure how to pass the data to each other unless we pass it to the parent and then back to the child.

I agree this is a little messy (and again reintroduces coupling with the navbar).

my concern here is with trying to limit dom effects that can be limited to vue to be limited to vue, to minimise side effects outside of vue. (as opposed to interfacing with <ul> / <li> in dropdowns - no choice there)

but introducing global state management for this seems overkill as well.. hmm.......

how about just a event subscribe / consume implementation?

// pagenavbutton / sitenavbutton subscribe('closeoverlays', () => this.show = false) publish('closeoverlays') // simple event implementation window.subscribers = [ ... ] ......... or just this https://developer.mozilla.org/en-US/docs/Web/Guide/Events/Creating_and_triggering_events

this way the dom manipulation stays within the subscriber component (page / site button); won't need to tie the implementation to the classes and ids as well

Thanks for the suggestion 😄

ang-zeyu · 2021-02-03T07:20:27Z

packages/vue-components/src/Navbar.vue

+      this.isNavMenuShowing = false;
+    },
+    toggleNavMenu() {
+      if ((this.hasPageNav && window.innerWidth < 1300) 


how about (to avoid coupling this slot showing with the buttons):

let the button components define their own window.innerWidth limits, and destroy themselves upon exceeding this limit (or show themselves)

let navbar do a generic check for whether there is any html within the div you added above (then this slot can also be used for non-button content)

Shifted this to their individual component. navbar should be free of page/site-nav-button components other than the fallback content in the slot.

ang-zeyu · 2021-02-03T07:24:16Z

packages/vue-components/src/Navbar.vue

-        <span class="navbar-toggler-icon"></span>
-        <slot name="collapse"></slot>
-      </button>
+  <div ref="navbarContainer">


what's this ref for?

Oops it's not in used anymore 😬. Has been removed.

ang-zeyu · 2021-02-03T07:41:48Z

packages/vue-components/src/PageNavButton.vue

+    };
+
+    const $el = $(this.$refs.pageNavMenu);
+    const pageNav = document.getElementById('page-nav');


cloning the html directly would disable reactivity / other things if the site-nav / page-nav has reactive elements that have already been processed in some way (e.g. vue components)

you can refer to retriever.vue (used in <panel src="...">) for mounting a clone of the original html source (as received by the browser).
However, it makes a webrequest (although it would likely hit the cache), so if there's a 'native' method to retrieve the original page source, all the more better. If not, directly reusing <retriever> should be fine too.

Alright I've decided to use <retriever> to get the contents of the identifier id=page/site-nav. However, if these identifiers are not present, will have to look for their respective classes still so it will fallback to using appendChild. Not sure if that's fine?

ang-zeyu · 2021-02-03T07:44:40Z

packages/core-web/src/styles/nav-menu-bar.css

@@ -0,0 +1,39 @@
+.hide-nav-button {


how about: create a internal base <overlay> component, with these styles

use it in site/page-nav-button so we can scope these styles still, also to avoid repeating similar logic / template in both

Created an overlay component for both <page/site-nav-button>. Abstracted all the similar template and logic into this component.

jonahtanjz · 2021-02-05T04:37:15Z

@ang-zeyu Updated the PR accordingly. Would appreciate if you can look through it :)

ang-zeyu

Thanks, just a couple more nits:

ang-zeyu · 2021-02-05T14:04:31Z

packages/vue-components/src/Overlay.vue

+</script>
+
+<style scoped>
+    #toggle-site-nav-button {


let's rename these into classes since the ids are no longer necessary

Thanks! Updated :)

ang-zeyu · 2021-02-05T14:11:07Z

packages/vue-components/src/PageNavButton.vue

+  },
+  computed: {
+    showPageNav() {
+      return this.show && this.hasPageNav;


should it have || this.hasIdentifier as well?

Hmm actually this was implemented before but this will cause the page/site nav button to show up even if they are not present on the page. This happens when there is an identifier, for example, id=page-nav but the page does not have any headers, which will display an empty page nav menu when opened.

ahh I see. Thanks for clarifying!

ang-zeyu · 2021-02-05T14:11:38Z

packages/vue-components/src/SiteNavButton.vue

+  },
+  computed: {
+    showSiteNav() {
+      return this.show && this.hasSiteNav;


ang-zeyu · 2021-02-05T14:12:22Z

packages/vue-components/src/PageNavButton.vue

+<template>
+  <overlay
+    v-if="showPageNav"
+    :type="'pageNav'"


could do

Suggested change

:type="'pageNav'"

type="pageNav"

also for site nav, and :fragment

Thanks! Updated :)

ang-zeyu · 2021-02-05T14:18:53Z

packages/core-web/src/index.js

+  insertCss(`.nav-menu-open { max-height: calc(100% - ${headerHeight}px); }`);
+}
+
+function addResizeHeaderListener() {


shouhld we put this under detectAndApplyFixedHeaderStyles instead? (it has a guard for no fixed header as well)

Thanks! Shifted this into detectAndApplyFixedHeaderStyles.

ang-zeyu · 2021-02-05T14:42:03Z

packages/vue-components/src/Overlay.vue

@@ -0,0 +1,185 @@
+<template>
+  <div>
+    <span


suggestion: pull the icon upward using scoped slots, so the component specific styles can be moved up there

not sure if it would end up more verbose though 😐

Shifted this up. Doesn't look too verbose :)

ang-zeyu · 2021-02-05T14:45:09Z

packages/vue-components/src/Overlay.vue

+    }
+
+    .site-nav-menu,
+    .page-nav-menu {


could we combine these?

Thanks! Updated :)

ang-zeyu · 2021-02-05T14:48:27Z

docs/userGuide/syntax/navBars.mbdf

+To make these accessible on smaller screens, you can use the `<site-nav-button />` and `<page-nav-button />` components in the `lower-navbar` slot. By default, if the `lower-navbar` slot is not specified, both the site and 
+page navigation buttons will automatically be added if they exist.
+
+```html


let's disable the line numbers here

Alright updated :)

ang-zeyu · 2021-02-05T14:52:32Z

docs/userGuide/syntax/navBars.mbdf

+
+Component | Description
+--- | ---
+`page-nav-button` | Will pull everything wrapped with an identifier, `id=page-nav`, in the layout file into the menu. Alternatively, if the identifier is not present, it will pull the page navigation menu in automatically if it exist. 


let's shorten this

Suggested change

`page-nav-button` | Will pull everything wrapped with an identifier, `id=page-nav`, in the layout file into the menu. Alternatively, if the identifier is not present, it will pull the page navigation menu in automatically if it exist.

`page-nav-button` | Pulls any element with an identifier, `id=page-nav` into the menu. If no such element exists, it pulls any [page navigation menu](insert link here) used in the layout.

or anything else :-)

Thanks! Updated :)

ang-zeyu · 2021-02-05T14:53:49Z

packages/core-web/src/styles/markbind.css

@@ -120,7 +120,7 @@ code.hljs.inline {
 header[fixed] {
    position: fixed;
    width: 100%;
-    z-index: 1000;


One more minor styling issue: (navbar example in ug)

Thanks! Fixed :)

jonahtanjz · 2021-02-06T05:59:48Z

@ang-zeyu Thanks for the review! The PR is updated accordingly except for the part regarding the || this.hasIdentifier in page/site-nav-button component :)

ang-zeyu · 2021-02-06T10:31:43Z

packages/vue-components/src/Navbar.vue

  },
  beforeDestroy() {
    $('.dropdown', this.$el).off('click').offBlur();
    if (this.slots.collapse) $('[data-toggle="collapse"]', this.$el).off('click');
+    $(window).off();


let's make it specific, so other listeners don't get removed

Alright thanks. Updated :)

ang-zeyu · 2021-02-06T10:45:03Z

packages/vue-components/src/PageNavButton.vue

+  },
+  computed: {
+    showPageNav() {
+      return this.show && this.hasPageNav;


ahh I see. Thanks for clarifying!

ang-zeyu · 2021-02-06T10:48:56Z

packages/vue-components/src/Navbar.vue

@@ -52,11 +60,17 @@ export default {
      default: 'sibling-or-child',
    },
  },
+  provide() {
+    return {
+      toggleLowerNavbar: this.toggleLowerNavbar,


how about just a prop? since they're in the same compilation scope

Hmm just to clarify, if this is passed to <page/site-nav-button> component through a prop, since lower-navbar is a slot, if a user defines new content in the layout file for this slot without specifying the prop, for example,

<div slot="lower-navbar"> <site-nav-button /> </div>

then it won't be passed down to the component in this case?

#1445 (comment)

ang-zeyu · 2021-02-06T11:38:05Z

packages/vue-components/src/PageNavButton.vue

+  },
+  provide() {
+    return {
+      getNavMenuContent: this.getPageNavContent,


sorry, commented on the wrong line 🙊

Thanks for the quick clarification 👍. Updated :)

jonahtanjz · 2021-02-06T12:32:25Z

@ang-zeyu Thanks for the review 👍. PR is updated accordingly :)

ang-zeyu

Lgtm! 👍

Let's organise the commits if that's ok with you (just separate addResizeHeaderListener will do, with suitable commit titles / messages)

Implement support for site and page navigation menus on smaller screens

Note the hard limit for the merge / squash commit title is 72 - 8 characters (brackets and pr number github appends)

While not necessary, try to keep < 50 characters too (succinct titles go a long way in making the history easy to read)

E.g. Implement mobile site and page navigation menus
would do for the merge commit title

jonahtanjz · 2021-02-06T13:40:11Z

Thanks @ang-zeyu 👍

Have separated the commit for addResizeHeaderListener here 784c69c and updated the merge commit title.

ang-zeyu · 2021-02-06T13:52:33Z

Thanks @ang-zeyu 👍

Have separated the commit for addResizeHeaderListener here 784c69c and updated the merge commit title.

@jonahtanjz Thanks, sorry for the over simplification, you'll need to shift and squash the other ones into one commit too (I'm going to do a merge commit here)

When header size is changed, the respective css rules that depend on the height of the header remain the same. Let's modify the css rules to change according to the size of the header.

jonahtanjz · 2021-02-06T14:16:51Z

Oh right sorry about that 😅. Updated the commits :)

ang-zeyu reviewed Jan 15, 2021

View reviewed changes

jonahtanjz force-pushed the 980-make-pagenav-accessible branch from 04a4544 to fb628c9 Compare February 2, 2021 14:28

jonahtanjz marked this pull request as ready for review February 2, 2021 14:35

jonahtanjz changed the title ~~[WIP] Make navigation menus accessible on smaller screens~~ Make navigation menus accessible on smaller screens Feb 2, 2021

ang-zeyu reviewed Feb 3, 2021

View reviewed changes

ang-zeyu reviewed Feb 5, 2021

View reviewed changes

ang-zeyu reviewed Feb 6, 2021

View reviewed changes

ang-zeyu approved these changes Feb 6, 2021

View reviewed changes

jonahtanjz force-pushed the 980-make-pagenav-accessible branch 2 times, most recently from 81c666e to b5d7f2b Compare February 6, 2021 13:35

Make css with header height reactive

433ca98

When header size is changed, the respective css rules that depend on the height of the header remain the same. Let's modify the css rules to change according to the size of the header.

Implement mobile site and page navigation menus

732c39d

jonahtanjz force-pushed the 980-make-pagenav-accessible branch from b5d7f2b to 732c39d Compare February 6, 2021 14:14

ang-zeyu added this to the v3.0 milestone Feb 6, 2021

ang-zeyu merged commit 6e5f335 into MarkBind:master Feb 6, 2021

jonahtanjz mentioned this pull request Apr 30, 2021

Use portals for mobile site and page nav #1556

Merged

10 tasks

gerteck mentioned this pull request Feb 19, 2025

Edge case for page/site-nav display of additional content and heading #2606

Open

	`page-nav-button` \| Will pull everything wrapped with an identifier, `id=page-nav`, in the layout file into the menu. Alternatively, if the identifier is not present, it will pull the page navigation menu in automatically if it exist.
	`page-nav-button` \| Pulls any element with an identifier, `id=page-nav` into the menu. If no such element exists, it pulls any [page navigation menu](insert link here) used in the layout.

Make navigation menus accessible on smaller screens #1445

Make navigation menus accessible on smaller screens #1445

Conversation

jonahtanjz commented Jan 14, 2021 • edited Loading

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtanjz commented Jan 18, 2021 • edited Loading

ang-zeyu commented Jan 18, 2021 • edited Loading

damithc commented Jan 18, 2021

ang-zeyu commented Jan 18, 2021

damithc commented Jan 18, 2021

jonahtanjz commented Jan 18, 2021 • edited Loading

ang-zeyu commented Jan 19, 2021

damithc commented Jan 19, 2021

ang-zeyu commented Jan 19, 2021

jonahtanjz commented Jan 21, 2021

ang-zeyu commented Jan 21, 2021

damithc commented Jan 21, 2021

jonahtanjz commented Feb 2, 2021

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtanjz Feb 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtanjz commented Feb 5, 2021

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtanjz commented Feb 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtanjz commented Jan 14, 2021 •

edited

Loading

ang-zeyu Jan 15, 2021 •

edited

Loading

jonahtanjz commented Jan 18, 2021 •

edited

Loading

ang-zeyu commented Jan 18, 2021 •

edited

Loading

jonahtanjz commented Jan 18, 2021 •

edited

Loading

jonahtanjz Feb 4, 2021 •

edited

Loading