Support Page Navigation Sidebar by Chng-Zhi-Xuan · Pull Request #465 · MarkBind/markbind

Chng-Zhi-Xuan · 2018-12-12T07:42:03Z

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Documentation update
• [x] New feature

Resolves #219

What is the rationale for this request?
User would like to be able to use a page navigation component that is automatically generated from headings within the page.

Prototype 2 Demo

Prototype 3, Responsive Design Demo

To Do	Done?
Responsive design	✔️
Change specifying `pageNav` within `front-matter` from `Boolean` to `{1 ... 6} or default`	✔️
Add specifying `pageNavHeader` to `front-matter` as the header text for page navigation	✔️
Adaptive CSS if `site-nav` or `footer` is not specified in the page	✔️
Page navigation automatically scrolls to active heading as user scroll the page	✔️
Indentation of smaller headings	✔️
Include headings in `Panel headers` and `Panel contents` if expanded	✔️
Live preview via `markbind serve` re-inserts page navigation	✔️
Update test site to use page navigation	✔️
Change page navigation and scroll bar styling from default Bootstrap theme	✔️
Update documentation for new feature and using it	✔️
Support auto expanding and collapsing of panels upon clicking an anchor link	⚠️ On hold

References
Bootstrap Scrollspy
Custom Scrollbars
Scrollspy Event Handling

Testing instructions:

Use Netlify Preview
Go to the "Docs" tab then "Using Components" sub section and the page navigation bar should appear.

Chng-Zhi-Xuan · 2018-12-12T07:47:29Z

Discussion:

Currently page navigation is enabled via specifying a boolean value in frontmatter.
Via pageNav: true. Then it uses the headingIndexingLevel to select which headings are added to the navigation bar.

I was thinking if we should decouple it from headingIndexingLevel and
specify a set of numbers {1, ... 6} instead?
The page navigation allows for header text, Should this be specified in front matter as a string? But this will conflict with the first idea of specifying numbers.
Page navigation is inserted only after the page is generated (so as to capture included files). Thus every page navigation insertion requires a second file write, which may impact build speed.

Would this be acceptable performance wise?
Page navigation is inserted outside all content wrappers, with <div id='app'> being the parent. Will shift it inside, similar to site-nav if there are any issues found with responsive design.
Colour scheme, font size and styling suggestions are also welcome.

yamgent · 2018-12-12T12:28:48Z

Currently page navigation is enabled via specifying a boolean value in frontmatter. Via pageNav: true. Then it uses the headingIndexingLevel to select which headings are added to the navigation bar. I was thinking if we should decouple it from headingIndexingLevel and specify a set of numbers {1, ... 6} instead?

We can do that to allow for more flexibility. Maybe also allow a way for the author to specify a default for page navigation in site.json? Then the author can also use pageNav: default to both activate it and use the default heading level.

The page navigation allows for header text, Should this be specified in front matter as a string? But this will conflict with the first idea of specifying numbers.

Might seem a bit too arbitrary. Since we are going to implement indentation, allowing the author to specify to such extent might make it difficult to render the indentation level reasonably. Also it is very likely that the author might forget to update the list when changing the heading texts.

Page navigation is inserted only after the page is generated (so as to capture included files). Thus every page navigation insertion requires a second file write, which may impact build speed. Would this be acceptable performance wise?

I don't think that all pages will have page navigation activated, so I think it is fine for now to have a second pass, especially if it makes implementation way easier.

Page navigation is inserted outside all content wrappers, with <div id='app'> being the parent. Will shift it inside, similar to site-nav if there are any issues found with responsive design.

Colour scheme, font size and styling suggestions are also welcome.

The design looks fine atm, I understand the rationale for putting the page nav scrollbar on the left side, but I hope the user isn't confused by it. :P Or maybe hide the scroll bar, while still allowing the user to scroll it with the mouse scroll wheel?

Chng-Zhi-Xuan · 2018-12-12T12:58:10Z

We can do that to allow for more flexibility. Maybe also allow a way for the author to specify a default for page navigation in site.json? Then the author can also use pageNav: default to both activate it and use the default heading level.

Default value in site.json sounds good.

Might seem a bit too arbitrary. Since we are going to implement indentation, allowing the author to specify to such extent might make it difficult to render the indentation level reasonably. Also it is very likely that the author might forget to update the list when changing the heading texts.

The difficult part about indentation comes from having the headings in the right order. I am referring that this current automatic page navigation doesn't allow the user to put some text above the heading links. It is simple to implement as there is already <a class="navbar-brand" href="#">Page Nav Header</a> implemented in Bootstrap to put non href text within the top of the page nav bar.

Getting the header string is the difficult part if we were to implement the number + default syntax mentioned above. Just checking default is easy, using .trim() and ===. Likewise for single numbers.

I think we just need to choose between 2 different implementation choices for header string

Specify a second variable in frontmatter, like pageNavHeader: "My page nav header".
Number + Header string. What about this syntax pageNav: 3, "My page nav header" or pageNav: default, "My page nav header".

I don't think that all pages will have page navigation activated, so I think it is fine for now to have a second pass, especially if it makes implementation way easier.

👍

The design looks fine atm, I understand the rationale for putting the page nav scrollbar on the left side, but I hope the user isn't confused by it. :P Or maybe hide the scroll bar, while still allowing the user to scroll it with the mouse scroll wheel?

This was intentional as I felt having 2 scrollbars (page scroll) adjacent to each other was an eyesore. Hiding it would result in users perceiving that it is not scroll-able. A compromise is to lower the font size so more can fit within the 80vh height I've set.

A more extreme alternative would be having a dynamic font size such that all the headings fit without needing to scroll (but may render the headings unreadable if there are too many)

damithc · 2018-12-12T13:47:20Z

This page has a well-done site-nave and page-nav if you need some inspiration :-) https://angular.io/guide/architecture-components

yamgent · 2018-12-12T14:45:14Z

Specify a second variable in frontmatter, like pageNavHeader: "My page nav header".

Oh I see. Second variable seems OK? If the author does not specify, it can be the name of the page itself.

This was intentional as I felt having 2 scrollbars (page scroll) adjacent to each other was an eyesore. Hiding it would result in users perceiving that it is not scroll-able.

Hmm, looking at the Angular documentation for inspiration. :) The page nav scrollbar for the Angular documentation is on the right side, but it is of a different color compared to the actual page's scrollbar. It is also thinner than the default OS scrollbar. That style might be worth considering as well?

Chng-Zhi-Xuan · 2018-12-14T07:49:40Z

Update:

Prototype 2 is up, updated To do and Demo in PR comment
- Used .small class on #pageNav to fit more headings
- Indentation of smaller headings is +5% margin of its immediate parent
Removed headings from Panel headers in component documentation as it cluttered the page navigation severely with buggy behaviour (Bootstrap Scrollspy skips when "spied" elements are very close together)

yamgent

Nice. 👍 I like the indentation, they look quite nice.

Got some issues I noticed in the meantime though:

When doing markbind serve live preview, if the original document is modified, the page nav disappears when MarkBind rebuilds the page.
When selecting the items at the bottom of the page nav list (e.g. Advanced Tips and Tricks), the page nav "jumps". This looks quite disorienting to the user.

yamgent · 2018-12-14T13:08:19Z

+    max-height: 80vh;
+    position: fixed;
+    width: 300px;
+    right: 0;


Remember to sort the attributes in alphabetical order.

Chng-Zhi-Xuan · 2018-12-14T13:24:56Z

When doing markbind serve live preview, if the original document is modified, the page nav disappears when MarkBind rebuilds the page.

This is due to having page navigation inserted only after a page has fully generated, and called only once within site.js, similar to collectHeadingsAndKeywords. Might look for a solution while working on the other To Dos

When selecting the items at the bottom of the page nav list (e.g. Advanced Tips and Tricks), the page nav "jumps". This looks quite disorienting to the user.

Unfortunately smooth scrolling is not available on some browsers (when using scrollIntoView) including Chrome, then it defaults to instant scroll 😞. Furthermore, Bootstrap's scrollspy is quite trigger happy when the page is scrolled all the way to the bottom as well, often skipping some headings (causing "jumps" within the list).

Will look into improving this ... if it is possible.

damithc · 2018-12-22T03:39:03Z

@Chng-Zhi-Xuan What's the planned behavior for preloaded but collapsed sections? For dynamically-loaded sections?

Chng-Zhi-Xuan · 2018-12-22T04:07:12Z

@Chng-Zhi-Xuan What's the planned behavior for preloaded but collapsed sections? For dynamically-loaded sections?

@damithc , currently it is hard coded to not index content within any collapsed panels.

It is easy to index it as the content is already there in the raw html file, however the href upon clicking would not work since it is hidden by vuestrap upon loading the page. The href will work once any of the content is visible on the page (expanded panel).

I wanted to avoid having the users click on a href and wondering why it doesn't jump to that section.

What I may explore is maybe have the page navigation dynamically change depending on which headers are visible at any given point in time. However it is most likely difficult to code as well as having performance issues on deeply nested panels having its contents revealed in 1 click.

damithc · 2018-12-22T04:40:23Z

I wanted to avoid having the users click on a href and wondering why it doesn't jump to that section.

What if the panel is expanded by default but collapsed by the user?

What I may explore is maybe have the page navigation dynamically change depending on which headers are visible at any given point in time. However it is most likely difficult to code as well as having performance issues on deeply nested panels having its contents revealed in 1 click.

Sure, we can omit that from the first version.

Chng-Zhi-Xuan · 2018-12-22T04:43:46Z

What if the panel is expanded by default but collapsed by the user?

If the panel is expanded, its header + children content get indexed, including any child panels with headings. The logic is simplified to (If my parent component is a panel and it is not expanded, do not index). Upon collapsing by the user, the header link stays there but would not be able to jump to that header.

The current planned behaviour is indexing any visible content upon loading the page, and not changing it if certain contents become visible / invisible.

acjh · 2018-12-22T04:50:05Z

We could expand panels based on window.location.hash, so it works even when sharing links.

scrollToUrlAnchorHeading in setup.js
window.onhashchange

Chng-Zhi-Xuan · 2018-12-22T04:59:47Z

We could expand panels based on window.location.hash, so it works even when sharing links.

To clarify, you are suggesting that hidden headings within any panels will be expanded using a javascript function, upon clicking a href whose heading is not yet visible?

Might be tricky given that panels can be deeply nested. Also need to test behaviour with Bootstrap's ScrollSpy.

acjh · 2018-12-22T05:05:20Z

To clarify, you are suggesting that hidden headings within any panels will be expanded using a javascript function, upon clicking a href whose heading is not yet visible?

Correct.

Chng-Zhi-Xuan · 2018-12-27T09:01:41Z

Update:

Updated PR description To-Dos and added responsive design demo.

yamgent

Sorry for the late reply, I checked the latest one out, looks pretty good. 👍

Some comments on the code (I assume that it is more stabilized at this stage, but if the remaining features requires significant revamp, then the comments may not be appropriate now):

yamgent · 2018-12-28T13:54:21Z

+            // Increase nesting level by 1
+            headingHTML += nestedHeadingHTML;
+          } else {
+            headingHTML += currentHeadingHTML;


There seems to be a mixture of determining how to partition the headings, and actual generation of the HTML code. Is it possible to separate the two? Otherwise the function gets really hard to read.

The actual HTML generation needs to be concurrent with partitioning. It builds it from top to bottom while maintaining the stack.

I guess I can apply SLAP and bring the HTML generation to another function, similar to formatSiteNav?

I guess I can apply SLAP and bring the HTML generation to another function, similar to formatSiteNav?

I think that's a good place to start. We can see if there's any further improvements to be made after following that.

Chng-Zhi-Xuan · 2019-01-04T12:14:04Z

Update:

Rebase to latest master, squashed commits
Update test site to use page navigation
PR description To-Dos updated
Documentation still in progress
Encountering some issues in implementing Support auto expanding and collapsing of panels upon clicking an anchor link, this functionality might be pushed to another PR.

Discussion:

Regarding the issue of whether page navigation exists in a layout. Currently to activate the page navigation feature, the user needs to input a number or default within the frontmatter of a page.

However, the layouts are file specific. So is there a good case to have a stand-alone file to have page navigation supported in layouts? Since the file will only have 2 lines (heading level and page navigation header text).

damithc · 2019-01-04T12:37:39Z

However, the layouts are file specific. So is there a good case to have a stand-alone file to have page navigation supported in layouts? Since the file will only have 2 lines (heading level and page navigation header text).

Introduce a new file e.g., settings.md into layouts that can contain this kind of settings?

Chng-Zhi-Xuan · 2019-01-05T03:59:24Z

Introduce a new file e.g., settings.md into layouts that can contain this kind of settings?

To extend your idea, currently the layout files for footer and other items are separated. It would be neater if all the different files (footer, siteNav, css) are all stored in their respective folders and have a simple settings.md file to "store" the file names required. Basically abstracting layouts to just a front matter file.

E.G, content within settings.md:

css: colorTheme1.css, blackTheme2.css
footer: simpleFooter.md
pageNav: default
pageNavHeader: "This is a list of stuff"
siteNav: mySiteNav.md
script: myScript1.js, myScript2.js

Then all the different file types are just stored in their respective folders within _/markbind

damithc · 2019-01-05T04:05:31Z

Then all the different file types are just stored in their respective folders within _/markbind

That way the same files can be used in different layouts right? That would be highly desirable.

Chng-Zhi-Xuan · 2019-01-09T09:44:47Z

Update:

Ready for review
Updated PR description To Dos
Added new slim-scroll class to markbind.css
Added documentation for page navigation within Tweaking The Page.
Using Components section will have a scroll-able page nav and Tweaking The Page section will have a non-scroll-able page nav. Please take a look using Netlify Preview.

Bug:

The scollspy functionality skips headings if there are HTML tags within the headings.
- Within Tweaking The Page section, the heading with <head> enclosed within a <code> tag will not show up as active.

Discussion

For the bug above, it most likely can't be fixed and would require the author to avoid having tags within headings. Would that be OK?
Would like reviewers to focus on the styling of the new scrollbar (which can be adopted within any scroll-able HTML element) as well as the page navigation .active heading style.

damithc · 2019-01-09T10:47:36Z

Nice progress!

For the bug, check if a heading with a font icon is affected. If it is, it's a significant problem because users can use font icons in headings often. Also, how about things like these? # My heading %%(contd)%%

Use font color (rather than vertical bars) to indicate selected heading? As we already have two scroll bars on either side, using vertical bars to show current heading could become a case of 'too many vertical bars'? Alternatively, we can use dot or something.

Should we move the scrollbar to be on the other side? It is true that having two scrollbars near to each other is a problem. Having one in the middle of the page doesn't look nice too, especially because the current one does attract a significant amount of attention. Alternativley, we can try to make it even thinner/lighter but not sure if that will affect the usability.

Overall though, we can release the feature as soon as the functionality is correct and good enough. UI tweaks can be done in future releases. Again, good work!

yamgent

Apologies for the late review, I think just two minor nits left, you can start squashing the commits already since it mostly seems fine now.

yamgent · 2019-01-17T03:04:12Z

+    -webkit-transition: 0.4s;
+}
+
+#page-nav a:link, #page-nav a:visited {


Put it in two lines, like so:

#page-nav a:link, #page-nav a:visited {

yamgent · 2019-01-17T03:15:59Z

+  $('modal').remove();
+  $(elementSelector).each((i, elem) => {
+    // Check if heading or panel is already inside an unexpanded panel
+    let insideUnexpandedPanel = false;


insideUnexpandedPanel -> isInsideUnexpandedPanel (to make it clearer that the variable is a boolean).

Chng-Zhi-Xuan · 2019-01-18T04:31:06Z

Update:

Applied changes requested.
Squashed commits.

Upon further discussion, got future improvements possible. :P

Chng-Zhi-Xuan · 2019-01-19T07:10:22Z

Update:

Re-ordered logic to insert page navigation within Page.generate. This is to avoid a double file write in previous implementation (called insertPageNav within Site.updateSiteData).

Notes:

Page navigation is unable to index headings from included sources within 1 pass.
(specific issue for expanded panels with src).

Implementation to support this should be done in another PR.

yamgent · 2019-01-19T07:22:35Z

+    <nav id="page-nav" class="navbar navbar-light bg-transparent slim-scroll">
+  <a class="navbar-brand" style="white-space: inherit; color: black" href="#">Testing Page Navigation</a>
+  <nav class="nav nav-pills flex-column my-0 small" style="flex-wrap: nowrap;">
+    <a class="nav-link py-1" href="#navigation">Navigation&#x200E;</a>


As discussed, this heading (a site nav) and the footer heading shouldn't be appearing. :P

Chng-Zhi-Xuan · 2019-01-19T08:04:24Z

Update:

Fixed indexing headers in html tags outside content-wrapper

yamgent · 2019-01-19T08:56:53Z

As spoken, have to rebase this PR now with the new anchor generated ID, we should have no problems with the pesky <, > and other weird symbols now.

Chng-Zhi-Xuan · 2019-01-21T04:18:07Z

Update:

Rebased to latest master
Anchor IDs now generate properly and does not introduce any "skipping" behaviour within Scrollspy.

damithc · 2019-01-21T05:50:02Z

Anchor IDs now generate properly and does not introduce any "skipping" behaviour within Scrollspy.

That's great news! 💯

Chng-Zhi-Xuan added the s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding label Dec 12, 2018

Chng-Zhi-Xuan self-assigned this Dec 12, 2018

Chng-Zhi-Xuan requested a review from yamgent December 12, 2018 07:42

Chng-Zhi-Xuan changed the title ~~[WIP, Prototyping] Support Page Navigation Sidebar~~ [WIP] Support Page Navigation Sidebar Dec 14, 2018

yamgent reviewed Dec 14, 2018

View reviewed changes

yamgent reviewed Dec 28, 2018

View reviewed changes

Chng-Zhi-Xuan force-pushed the 219-page-navigation branch from c43766e to 40d5916 Compare January 4, 2019 12:06

Chng-Zhi-Xuan mentioned this pull request Jan 5, 2019

Allow reusing of files in page layouts #539

Closed

Chng-Zhi-Xuan changed the title ~~[WIP] Support Page Navigation Sidebar~~ Support Page Navigation Sidebar Jan 9, 2019

yamgent self-requested a review January 15, 2019 03:38

yamgent requested changes Jan 17, 2019

View reviewed changes

Chng-Zhi-Xuan force-pushed the 219-page-navigation branch from 930b577 to 896a95a Compare January 18, 2019 04:30

yamgent previously approved these changes Jan 19, 2019

View reviewed changes

Chng-Zhi-Xuan force-pushed the 219-page-navigation branch from 896a95a to 80ae39e Compare January 19, 2019 07:07

yamgent reviewed Jan 19, 2019

View reviewed changes

Chng-Zhi-Xuan force-pushed the 219-page-navigation branch from 80ae39e to 969dbf4 Compare January 19, 2019 08:01

yamgent approved these changes Jan 19, 2019

View reviewed changes

Chng-Zhi-Xuan added 9 commits January 21, 2019 11:54

Asset: Add Bootstrap Javascript Utilities

bbef2c1

Asset: Add page navigation CSS file

b7ce08d

Asset: Add slim-scroll class

dec6c37

Asset: Add auto scroll function to page nav

4b06cc1

Add page navigation feature

c2d2c6a

Docs: Add page navigation to documentation

377b34f

Test: Update Site unit tests

4508944

Test: Add page navigation to test site

c7722c9

Test: Update expected files folder

9a20ab5

Chng-Zhi-Xuan force-pushed the 219-page-navigation branch from 969dbf4 to 9a20ab5 Compare January 21, 2019 04:10

yamgent approved these changes Jan 21, 2019

View reviewed changes

yamgent added this to the v1.17.0 milestone Jan 21, 2019

yamgent merged commit 2971394 into MarkBind:master Jan 21, 2019

Chng-Zhi-Xuan mentioned this pull request Jan 26, 2019

PageNav cannot handle duplicate headings #633

Closed

Chng-Zhi-Xuan deleted the 219-page-navigation branch May 21, 2019 06:40

Conversation

Chng-Zhi-Xuan commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Chng-Zhi-Xuan commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yamgent commented Dec 12, 2018

Uh oh!

Chng-Zhi-Xuan commented Dec 12, 2018

Uh oh!

damithc commented Dec 12, 2018

Uh oh!

yamgent commented Dec 12, 2018

Uh oh!

Chng-Zhi-Xuan commented Dec 14, 2018

Uh oh!

yamgent left a comment

Choose a reason for hiding this comment

Uh oh!

yamgent Dec 14, 2018

Choose a reason for hiding this comment

Uh oh!

Chng-Zhi-Xuan commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damithc commented Dec 22, 2018

Uh oh!

Chng-Zhi-Xuan commented Dec 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damithc commented Dec 22, 2018

Uh oh!

Chng-Zhi-Xuan commented Dec 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acjh commented Dec 22, 2018

Uh oh!

Chng-Zhi-Xuan commented Dec 22, 2018

Uh oh!

acjh commented Dec 22, 2018

Uh oh!

Chng-Zhi-Xuan commented Dec 27, 2018

Uh oh!

yamgent left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yamgent Dec 28, 2018

Choose a reason for hiding this comment

Uh oh!

Chng-Zhi-Xuan Dec 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamgent Jan 1, 2019

Choose a reason for hiding this comment

Uh oh!

Chng-Zhi-Xuan commented Jan 4, 2019

Uh oh!

damithc commented Jan 4, 2019

Uh oh!

Chng-Zhi-Xuan commented Jan 5, 2019

Uh oh!

damithc commented Jan 5, 2019

Uh oh!

Chng-Zhi-Xuan commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damithc commented Jan 9, 2019

Uh oh!

yamgent left a comment

Choose a reason for hiding this comment

Uh oh!

yamgent Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

yamgent Jan 17, 2019

Choose a reason for hiding this comment

Uh oh!

Chng-Zhi-Xuan commented Dec 12, 2018 •

edited

Loading

Chng-Zhi-Xuan commented Dec 12, 2018 •

edited

Loading

Chng-Zhi-Xuan commented Dec 14, 2018 •

edited

Loading

Chng-Zhi-Xuan commented Dec 22, 2018 •

edited

Loading

Chng-Zhi-Xuan commented Dec 22, 2018 •

edited

Loading

Chng-Zhi-Xuan Dec 31, 2018 •

edited

Loading

Chng-Zhi-Xuan commented Jan 9, 2019 •

edited

Loading

Chng-Zhi-Xuan commented Jan 18, 2019 •

edited

Loading

Chng-Zhi-Xuan commented Jan 19, 2019 •

edited

Loading