Use portals for mobile site and page nav #1556

ang-zeyu · 2021-04-30T04:54:02Z

What is the purpose of this pull request?

4th item in #1534 (comment)

Overview of changes:

Change the retrieval to use portals https://portal-vue.linusb.org/guide/getting-started.html

Setup portal sources at build time (for <site-nav> / {{pageNav}} / id="site/page-nav")
- These are transformed into an OverlaySource (new) vue component
Change Overlay retrieval to use portals instead, for <site-nav> / {{pageNav}} / id="site/page-nav"

Anything you'd like to highlight / discuss:

Testing instructions:
Manual testing needed

Proposed commit message: (wrap lines at 72 characters)
Use portals for mobile site and page nav

With Vue SSR implemented, site and page nav mobile overlays are unable
to retrieve the original page source describing the respective
navigation components.
This results in being unable to mount an interactive copy of said
components.

Let's change the mobile site and page nav overlays to use portals as
the content source instead.
Portal sources (site / page nav) are identified and created during
build time.

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 · 2021-04-30T05:03:49Z

@wxwxwxwx9 @jonahtanjz your reviews would be great here! Especially on possible alternative approaches.

Per #1534 (comment), the old retrieval method won't work with ssr as the page is ssr-rendered (template / app information is lost).

The overall approach is changed to use portals, powered by the portal-vue library.

The main upside would be no network requests => html parsing, etc.
The main downside would be more backend coupling.

Some alternatives considered:

Duplicate the html node during build time (and add v-pre on top), then use retriever.vue as before to retrieve that.
- should be straightforward to implement, but still results in some form of backend coupling. Also, increased html size.
Storing the site/page navs in separate files, then likewise using retriever.vue to retrieve
- backend coupling, file bloat.

Not too sure if there are methods to do this without backend coupling 🤔

jonahtanjz

Thanks @ang-zeyu!

Just a brief review. I believe using portals is a good approach. I do not have any objection with that. I've noticed some regressions with the current implementation regarding the site and page navigation.

jonahtanjz · 2021-04-30T06:04:54Z

packages/vue-components/src/PageNavButton.vue

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


I believe we need a separate variable to check for page nav as the identifier page-nav can exist without a page nav.
For exmaple,

<nav id="page-nav" class="fixed-header-padding"> <div class="nav-component slim-scroll"> {{ pageNav }} </div> </nav>

The nav element will always be present even if {{pageNav}} is empty.

This is causing the page-nav-button to show up on pages without page nav.

reverted temporarily

jonahtanjz · 2021-04-30T06:37:15Z

packages/vue-components/src/PageNavButton.vue

-    getPageNavContent() {
-      const wrapper = document.createElement('div');
-      const pageNavTitle = document.getElementsByClassName('page-nav-title')[0];
-      const pageNavLinks = document.getElementById('mb-page-nav');
-      if (pageNavTitle) {
-        wrapper.appendChild(pageNavTitle.cloneNode(true));
-      }
-      if (pageNavLinks) {
-        wrapper.appendChild(pageNavLinks.cloneNode(true));
-      }
-      return wrapper;
-    },


I believe this method is still needed so that we can still pull in the page nav if the page-nav identifier is not present? As discussed here, we should pull in the identifier first if they exist and if not, we should find the page nav and then pull that in.

Alternatively, we can also change the requirement such that users will have to explicitly specify the page nav with the identifier if that makes it better?

I believe this method is still needed so that we can still pull in the page nav if the page-nav identifier is not present? As discussed here, we should pull in the identifier first if they exist and if not, we should find the page nav and then pull that in.

I standardised this to a portal implementation as well

jonahtanjz · 2021-04-30T06:40:04Z

packages/vue-components/src/PageNavButton.vue

-    this.hasPageNav = document.getElementById('mb-page-nav') !== null;
+    if (document.getElementById('page-nav') !== null) {
+      this.portalName = 'page-nav';
+    } else if (document.getElementById('mb-page-nav') !== null) {


As mentioned above, page-nav if present will always exist. As mb-page-nav will only be present if a page nav is present, we can use this instead to check if the page-nav-button should be shown.

reverted temporarily

jonahtanjz · 2021-04-30T06:40:34Z

packages/vue-components/src/SiteNavButton.vue

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


Similar to page nav

also reverted temporarily

jonahtanjz · 2021-04-30T06:41:39Z

packages/vue-components/src/SiteNavButton.vue

-    getSiteNavContent() {
-      return document.getElementsByClassName('site-nav-root')[0];
-    },


Similar to page nav (Should pull the first site nav if the identifier is not present).

also now using the portal implementation (more important here as <site-nav> can contain interactive vue components)

jonahtanjz · 2021-04-30T06:42:01Z

packages/vue-components/src/SiteNavButton.vue

-    this.hasSiteNav = document.getElementsByClassName('site-nav-root').length !== 0;
+    if (document.getElementById('site-nav') !== null) {
+      this.portalName = 'site-nav';
+    } else if (document.getElementsByClassName('site-nav-root').length !== 0) {


Similar to page nav

reverted temporarily

jonahtanjz · 2021-04-30T06:43:31Z

packages/vue-components/src/Overlay.vue

-    const navMenu = this.$refs.navigationMenu;
-    const buildNav = (navMenuItems) => {
-      if (!navMenuItems) { return; }
-      for (let i = 0; i < navMenuItems.childNodes.length; i += 1) {
-        navMenu.appendChild(navMenuItems.childNodes[i].cloneNode(true));
-      }
-      this.navMenuLoaded();
-    };
-
-    if (!this.hasIdentifier) {
-      buildNav(this.getNavMenuContent());


Similar to the comment for page nav, we will need this to manually pull in the first site nav or the page nav if their identifiers are not present.

shifted to portals as well!

ang-zeyu · 2021-04-30T07:36:11Z

Thanks for the speedy review! @jonahtanjz

ahh, my bad, I understand why that part of the code was structured as such now 🤣; I also missed the root issue with #1445 (comment) you brought up previously.

I went with the spec described in the docs and the original post in implementing it this time.

i.e. using MarkBind's auto generated {{ pageNav }} isn't necessary, only <some-element id="page-nav">. (to allow more customisation)

This is causing the page-nav-button to show up on pages without page nav.

One option is to create new layouts in our docs without the empty id="page-nav" element, following the current spec. This seems like the most "correct" option but is a lot of work for the author (need to maintain and configure two layout sets)

Another is to add this to the spec. i.e. id="page-nav" must also contain {{ pageNav }}. This is convenient for the author, but might be a little restrictive.

Alternatively, we can also change the requirement such that users will have to explicitly specify the page nav with the identifier if that makes it better?

This is also an option, and can simplify the implementation quite a bit. The downside is the user would then have to be aware of the additional need for id="page-nav". Also, still need to maintain and configure two layout sets.

Likewise for the site-nav.

What do you think? @jonahtanjz

ang-zeyu · 2021-04-30T07:50:14Z

Case-by-case illustration:

Option 1: (my interpretation)

id="page-nav" contains {{ pageNav }} - mobile page nav shown
id="page-nav" is present but does not contain {{ pageNav }} - shown
id="page-nav" is absent, but {{ pageNav }} is present - shown

More work in that need to configure two layout sets.

Option 2: (your interpretation)

id="page-nav" contains {{ pageNav }} - shown
id="page-nav" is present but does not contain {{ pageNav }} - not shown
id="page-nav" is absent, but {{ pageNav }} is present - shown

More restrictive in that user must use MarkBind's auto generated {{ pageNav }}. Less work as only 1 layout is needed.

Option 3: (new suggestion above)

id="page-nav" contains {{ pageNav }} - shown
id="page-nav" is present but does not contain {{ pageNav }} - shown
id="page-nav" is absent, but {{ pageNav }} is present - not shown

Need to configure two layout sets. Simpler, implementation wise. Slightly less convenient (author must be aware of id="page-nav" restriction).

Likewise for the sitenav, but between id="site-nav" and <site-nav>

damithc · 2021-04-30T08:13:05Z

@ang-zeyu is there any change from the user's POV? Is there a corresponding update to the user guide?

ang-zeyu · 2021-04-30T08:43:27Z

The current PR goes by the user guide's description (option 1), but will need a restructure of the layouts (e.g. below).
i.e. separate layouts / configs for pages with or without pagenav is required.

e.g. (option 1)

// layoutWithPageNav.md
<include src="base.md">
<variable name="doAddPageNav>true</variable>
</include>

// layoutWithoutPageNav.md
<include src="base.md" />

//base.md
...
{% if doAddPageNav %}
<div id="site/page-nav">
... the mobile page / site-nav button shows up if this id is present ... regardless of what's contained inside
</div>
{% endif %}
...

The netlify preview here shows what happens if one layout is stuck to -- mobile page nav button is still present even with an empty {{ pageNav }}.

The previous implementation in #1445 abides by option 2 (I missed while reviewing the earlier PR); That will need a ug update of the feature description.

It is more convenient however; Case in point the entire user guide is currently using only one layout file (https://raw.githubusercontent.com/MarkBind/markbind/master/docs/_markbind/layouts/userGuide.md). This is possible because empty {{ pageNav }} renders into nothing, and the implementation detects that as "page nav is not present".

The downside is MarkBind's site/auto generated page-nav (<site-nav> / {{ pageNav }}) must be present.

ang-zeyu · 2021-04-30T08:48:58Z

One more alternative: Check if the id="page/site-nav" contains any text, if so, the mobile page/site-nav is shown.

This should bring the best of both options 1 & 2.
The obvious downside is that id="page/site-nav" must contain some text (which should be true 99% of the time), and this definition might be a little technical for the user

damithc · 2021-04-30T08:59:06Z

Not sure if I understand the picture fully. I'll leave you guys to consider the author's POV as well and choose the best option.

jonahtanjz · 2021-04-30T10:49:49Z

@ang-zeyu Option 1 seems to allow users to have more control and flexibility but also more things for them to take note of. This may be a little inconvenient on the user side?

One more alternative: Check if the id="page/site-nav" contains any text, if so, the mobile page/site-nav is shown.

This should bring the best of both options 1 & 2.
The obvious downside is that id="page/site-nav" must contain some text (which should be true 99% of the time), and this definition might be a little technical for the user

This alternative sounds good as it reduces the need for the user to do extra work. I think the benefits of abstracting this process from the user exceed the downside (since it only occurs in rare situation and if we are clear in the ug, should be sufficient).

ang-zeyu · 2021-04-30T12:54:33Z

Agreed!

I wonder if there's an even more intuitive solution than looking for text though. 🤔

Documenting for your mobile site / page navigation menu to show up, it must contain some text may seem very bewildering to the user.

It also dosen't take care of such cases nicely. In this case option 2 might be even better:

<nav id="page-nav">
  <h2>My custom page nav title!</h2>
  <img src="..." alt="accompanying image" />
  {{ pageNav }}
</nav>

Should we look <a /> tags instead?

So the documentation becomes:
If your site or page nav dosen't contain any anchor tags, MarkBind will automatically hide them for you.
Might still be a little odd though.

jonahtanjz · 2021-04-30T16:03:10Z

Should we look <a /> tags instead?

I think this is the most intuitive approach as a site nav and page nav should contain anchor tags. Having a mobile site nav or page nav that contains no links will not add much value to the users and will most likely not make a big difference if it is hidden from them.

So the documentation becomes:
If your site or page nav dosen't contain any anchor tags, MarkBind will automatically hide them for you.
Might still be a little odd though.

How about If your site or page nav dosen't contain any anchor tags links, MarkBind will automatically hide them for you.?
This will probably make it easier for the users to understand and also let them know that it works with Markdown's link syntax as well (as these will be converted to <a />).

ang-zeyu · 2021-04-30T16:23:23Z

👍, let's go with this then.

How about If your site or page nav dosen't contain any ~anchor tags~ links, MarkBind will automatically hide them for you.?
This will probably make it easier for the users to understand and also let them know that it works with Markdown's link syntax as well (as these will be converted to <a />).

oops, meant links 😅

I'll put this change up in a separate PR however so its clear this PR / commit is just for changing the background implemention.
It should be just be some extra checks surrounding these:

if (document.getElementById('site-nav') !== null && siteNavIdHasAnchorMagic) {
      this.portalName = 'site-nav';
} else if (document.getElementsByClassName('site-nav-root').length !== 0 && notNeeded) {
...

// similar for the page-nav branch

I'll add back in the {{ pageNav }} / <site-nav> checks in a while as well. (should be at the same location - siteNavIdHasAnchorMagic -> siteNavIdHasSiteNavComponentMagic)

jonahtanjz

Just found a few more edge cases which may be causing some issues.

jonahtanjz · 2021-05-01T03:52:11Z

packages/core/src/html/NodeProcessor.js

@@ -297,6 +297,8 @@ class NodeProcessor {

    this.postProcessNode(node);

+    addSitePageNavPortal(node);


I think this is not working when it is using the MarkBind's auto generated variable {{ pageNav }} without the identifier, id="page-nav".
For example:

<nav class="fixed-header-padding"> <div class="nav-component slim-scroll"> {{ pageNav }} </div> </nav>

The links do not appear in the mobile page nav. It's showing a blank page nav.

I believe this is due to adding the site and page nav portal before the page nav is built. Inside index.js of Page, the NodeProcessor's process method which contains addSitePageNavPortal is called before the buildPageNav method, which means it will not be picked up by the portals method. Perhaps can shift addSitePageNavPortal to after the page nav is built?

jonahtanjz · 2021-05-01T04:01:24Z

packages/vue-components/src/Overlay.vue

+        this.$nextTick(() => {
+          $(this.$refs.navMenuContainer).find('a').on('click', this.toggleNavMenu);
+        });


I've also noticed this strange behaviour where after clicking on a page nav's link, the overlay does not close.

For example on the Using Components page:

MarkBind.-.User.Guide_.Using.Components.-.Google.Chrome.2021-05-01.11-54-29.mp4

Notice how after clicking on some of the links, the url changes but the overlay does not close (only after 2-3 clicks then it closes). I'm not sure if this method is the one causing this issue.

ang-zeyu · 2021-05-01T07:47:21Z

Thanks for the catches!

I've fixed both issues -- the first was a problem with where overlay-source for the auto page nav should be added (fixed by shifting), the second was due to toggleNavMenu firing multiple times from the event handler (over-attached) (fixed by using the bubbled event instead).

I found another undiscussed point with the previous implementation around the forwarding of styles / attributes however.
Based on https://markbind-master.netlify.app/userguide/formattingcontents#line-breaks, the current behaviour is:

Tag names / classes / styles / attributes / ids are not forwarded for id="site-nav/page-nav", resulting in slightly different styling.
the auto generated page nav + site nav components are, however.

Adding 55cf54d alone which forwards said things makes the site nav go poof, as there is a media query for display: none (<998....px).
Adding reasonable overrides (619d775) fixes this to some extent.

I wonder if we should

Leave the current behaviour be, then update the ug with a warning to reflect this behaviour - since it would break some descendant selectors / the selected element the author wants to carry forward.
Or adopt 55cf54d & 619d775 in another PR, then update the ug with this behaviour. The author would then be expected to add extra styles with the .override class if he/she wants to override our reasonable defaults.

This can really go both ways.

e.g. With forwarding, our user guide adds a thin grey bar on the right as expected, but might not be suitable for the mobile site nav. The author then has to override this by compositing #site-nav.override { ... }

Without, it breaks descendent selectors and the actual element's selectors.
I would think its more difficult to add separate styles for the mobile version as well because the root element becomes "generic" (just a <div> without tag, id, etc. information)

Wdyt? @jonahtanjz

jonahtanjz · 2021-05-01T11:11:50Z

I wonder if we should

Leave the current behaviour be, then update the ug with a warning to reflect this behaviour - since it would break some descendant selectors / the selected element the author wants to carry forward.

Or adopt 55cf54d & 619d775 in another PR, then update the ug with this behaviour. The author would then be expected to add extra styles with the .override class if he/she wants to override our reasonable defaults.

I think going with forwarding would make more sense as this will preserve as much of the original navigations as possible. We can add more default overrides if needed (so normal users would need to do as little work as possible) and the more advance users can customise the styling etc.

I feel if we go without forwarding it will make it too restrictive and also break some of the selectors as mentioned. That's just my thought on this.

e.g. With forwarding, our user guide adds a thin grey bar on the right as expected, but might not be suitable for the mobile site nav. The author then has to override this by compositing #site-nav.override { ... }

We can also remove this for the mobile navigations by default.

jonahtanjz · 2021-05-01T11:19:48Z

packages/vue-components/src/Overlay.vue

-    toggleNavMenu() {
-      if (!this.show) {
+    toggleNavMenu(ev) {
+      if (ev.target.tagName.toLowerCase() === 'a' || this.show) {


Suggested change

if (ev.target.tagName.toLowerCase() === 'a' || this.show) {

if (ev.target.tagName.toLowerCase() === 'a' && this.show) {

I think this should be && so that the site nav title does not close the overlay when it is clicked?

MarkBind.-.User.Guide.-.Getting.Started.-.Google.Chrome.2021-05-01.19-11-21.mp4

Fixed! Still ||, but added some extra checks, && would disable the closing button.

ang-zeyu · 2021-05-01T12:02:16Z

I feel if we go without forwarding it will make it too restrictive and also break some of the selectors as mentioned. That's just my thought on this.

👍, that's my preference as well. I've reverted those two commits for moving to another pr.

The auto generated site/pagenav without the wrapper id=site/page-nav will temporarily look a little strange pending this forwarding fix.

jonahtanjz

Just one more question. Other than that LGTM 👍

jonahtanjz · 2021-05-01T12:16:22Z

docs/_markbind/layouts/userGuide.md

@@ -2,6 +2,7 @@

 <div id="flex-body">
  <nav id="site-nav" class="fixed-header-padding">
+<panel header="To test interactivity">Expanding and closing should work</panel>


Should this be removed?

almost forgot 😅

jonahtanjz

LGTM 👍

Use portals for mobile site and page nav

94358f7

ang-zeyu requested review from wxwxwxwx9 and jonahtanjz April 30, 2021 04:54

ang-zeyu added 3 commits April 30, 2021 13:13

Re-add missing toggleLowerNavbar injection

0824305

Add temporary snippet for testing interactivity

6df94f1

Update expected test files

881b287

jonahtanjz reviewed Apr 30, 2021

View reviewed changes

ang-zeyu requested a review from damithc April 30, 2021 07:50

Restore restriction on site / page nav

9f9690e

jonahtanjz reviewed May 1, 2021

View reviewed changes

ang-zeyu added 6 commits May 1, 2021 14:31

Add missing page nav source

0880119

Forward static class for overlay-source to the portal-ed content

55cf54d

Fix multiple clicks for page nav to close

0934287

Update expected test files

39ce8c8

add reasonable overrides

619d775

Small renaming

26c4385

jonahtanjz reviewed May 1, 2021

View reviewed changes

ang-zeyu added 2 commits May 1, 2021 19:44

Further constrain overlay closing checks

d101d71

Revert style and tag forwarding

5e2de17

jonahtanjz reviewed May 1, 2021

View reviewed changes

Remove testing snippet

18a0a0d

jonahtanjz approved these changes May 1, 2021

View reviewed changes

jonahtanjz merged commit 42503a8 into MarkBind:master May 1, 2021

This was referenced May 2, 2021

Show mobile navigation menus only if links are present #1561

Merged

Forward root element styles to mobile navigation menu #1562

Merged

jonahtanjz added this to the v3.0 milestone May 6, 2021

		@@ -297,6 +297,8 @@ class NodeProcessor {

		this.postProcessNode(node);

		addSitePageNavPortal(node);

	if (ev.target.tagName.toLowerCase() === 'a' \|\| this.show) {
	if (ev.target.tagName.toLowerCase() === 'a' && this.show) {

Use portals for mobile site and page nav #1556

Use portals for mobile site and page nav #1556

Conversation

ang-zeyu commented Apr 30, 2021 • edited Loading

ang-zeyu commented Apr 30, 2021 • edited Loading

jonahtanjz 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

jonahtanjz Apr 30, 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

ang-zeyu commented Apr 30, 2021

ang-zeyu commented Apr 30, 2021 • edited Loading

damithc commented Apr 30, 2021

ang-zeyu commented Apr 30, 2021

ang-zeyu commented Apr 30, 2021

damithc commented Apr 30, 2021

jonahtanjz commented Apr 30, 2021

ang-zeyu commented Apr 30, 2021 • edited Loading

jonahtanjz commented Apr 30, 2021

ang-zeyu commented Apr 30, 2021

jonahtanjz left a comment

Choose a reason for hiding this comment

jonahtanjz May 1, 2021 • edited Loading

Choose a reason for hiding this comment

jonahtanjz May 1, 2021 • edited Loading

Choose a reason for hiding this comment

ang-zeyu commented May 1, 2021

jonahtanjz commented May 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu commented May 1, 2021

jonahtanjz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtanjz left a comment

Choose a reason for hiding this comment

ang-zeyu commented Apr 30, 2021 •

edited

Loading

ang-zeyu commented Apr 30, 2021 •

edited

Loading

jonahtanjz Apr 30, 2021 •

edited

Loading

ang-zeyu commented Apr 30, 2021 •

edited

Loading

ang-zeyu commented Apr 30, 2021 •

edited

Loading

jonahtanjz May 1, 2021 •

edited

Loading

jonahtanjz May 1, 2021 •

edited

Loading

jonahtanjz commented May 1, 2021 •

edited

Loading