-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support site navigation bar #259
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start in getting the site navigation to work. Some comments.
lib/Site.js
Outdated
+ '</frontmatter>\n\n' | ||
+ '# Hello world\n' | ||
+ 'Welcome to your page generated with MarkBind.\n'; | ||
|
||
const SITE_NAV_DEFAULT = '<markdown>\n' | ||
+ '- [Home {{glyphicon_home}}](/index.html)\n' | ||
+ '- Dropdown title :pencil2:\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the list should only just show Home
and nothing else? Won't make sense to list other "pages" when there is only index.html.
lib/Page.js
Outdated
@@ -9,15 +9,27 @@ const Promise = require('bluebird'); | |||
|
|||
const FsUtil = require('./util/fsUtil'); | |||
const logger = require('./util/logger'); | |||
const md = require('./markbind/lib/markdown-it'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be below MarkBind
to follow alphabetical order.
lib/Page.js
Outdated
if (listItems.length === 0) { | ||
return renderedSiteNav; | ||
} | ||
// Inline style |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Tidy up the style of the list item
Reason: Comments should specify what the change is about, rather than how the change is done (in-lining the style)
lib/Page.js
Outdated
if ($(this).children().first().is('ul')) { // Found nested list | ||
const nestedList = $(this).children().first(); | ||
const dropdownTitle = $(this).contents().first().text(); | ||
$(this).contents().first().remove(); // Remove nested list title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Replace the title with the dropdown wrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And avoid same-line comments.
lib/Page.js
Outdated
siteNavDataSelector('markdown').replaceWith(formattedSiteNav); | ||
|
||
const wrappedSiteNav = `<div id="${SITE_NAV_ID}">\n${siteNavDataSelector.html()}\n</div>`; | ||
const wrappedPageData = `<div id="${PAGE_CONTENT_ID}">\n${pageData}\n</div>`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, maybe this portion can be simplified by declaring the divs inside page.ejs
instead of having to figure out how to insert them manually. That way the code will be slightly simpler too.
Possible example of how this could be done:
<% if (siteNav) %><div id="nav"><%- siteNavContent %></div><% } %>
<%- content %>
asset/css/markbind.css
Outdated
border-right: 1px solid lightgrey; | ||
display: inline-block; | ||
max-height: 80vh; | ||
font-size: 14pt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Watch out for alphabetical order (some of the other ones below also have this issue).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a huge section of styles. Is this from a source? Consider a separate stylesheet.
docs/userGuide/contentAuthoring.md
Outdated
</li> | ||
</ul> | ||
|
||
The site navigation bar has [responsive properties](https://www.w3schools.com/html/html_responsive.asp), and will collapse to a menu button when the screen width is smaller than 767 pixels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be smaller than 768 pixels (the hiding triggers when it is 767 pixels).
asset/js/setup.js
Outdated
const dropdownContent = this.nextElementSibling; | ||
if (dropdownContent.style.maxHeight === '0px' | ||
|| !dropdownContent.style.maxHeight) { | ||
dropdownContent.style.transition = 'max-height 0.25s ease-in'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed, these can actually be put inside the .css file instead of using inline styles. Then you just need to modify the sibling's class instead of the sibling's inline style.
docs/userGuide/userQuickStart.md
Outdated
@@ -132,3 +133,5 @@ By default, it will try to push everything in the generated site (default dir: ` | |||
More details of deployment setting could be found in [here](ghpagesDeployment.html). | |||
|
|||
</div> | |||
|
|||
<include src="../_markbind/footers/userGuideSections.md" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't have this line since the footer is already specified in the frontmatter. Anyway we are getting rid of userGuideSections.md
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftovers from rebasing master
- Deploying a Site | ||
- <a href="{{baseUrl}}/userGuide/ghpagesDeployment.html">Github Pages Deployment</a> | ||
- <a href="{{baseUrl}}/userGuide/netlifyPreview.html">Preview site on Netlify</a> | ||
</markdown> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question: why are we not using markdown syntax for links here? Also, let's use * item
syntax for lists, to be consistent with other places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, I tested it using markdown syntax works fine. Will change it.
Also, let's use * item syntax for lists, to be consistent with other places.
👍
docs/userGuide/contentAuthoring.md
Outdated
- Nested Dropdown title :triangular_ruler: | ||
- Nested Dropdown item one | ||
- Nested Dropdown item two | ||
- Dropdown item two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have items without links here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It serves as a working example of how the page navigation is rendered. Much like how the default content for footers have This is a dynamic height footer .....
for the Author to edit out after seeing how it is rendered.
Might rename the list items to be [DEMO] Dropdown title / item
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... it is a bit misleading to give examples like this one as this one will not work as expected (due to missing links).
- <a href="{{baseUrl}}/userGuide/developingASite.html">Developing a Site</a> | ||
- <a href="{{baseUrl}}/userGuide/siteConfiguration.html">Site Configuration</a> | ||
- <a href="{{baseUrl}}/userGuide/contentAuthoring.html">Content Authoring</a> | ||
- <a href="{{baseUrl}}/userGuide/includingContents.html">Including Contents</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A new page "Known Problems" have been added in a recent PR, do add that in as well.
A side nav bar that has a grey theme https://www.berriart.com/sidr |
docs/userGuide/contentAuthoring.md
Outdated
- More than one navigation file can be created for different pages. | ||
|
||
- Author your navigation layout using Markdown's unordered list syntax as shown below. | ||
- When you are using links (`[linkName](url)`), ensure that the url starts from the root directory `/` when you are referencing a page in your site. e.g. `/folder1/myPage.html` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this works on GitHub pages? Should it not be {{baseUrl}}/folder1/myPage.html
?
docs/userGuide/contentAuthoring.md
Outdated
<ul style="list-style-type: none; margin-left:-1em"> | ||
<li>Dropdown item one</li> | ||
<li><button class="dropdown-btn">Nested Dropdown title 📐 | ||
</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What went on here? (Even if this is really the case, we can put this on the previous line.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the raw HTML generated and copy pasted. Will format it nicely inside docs.
docs/userGuide/contentAuthoring.md
Outdated
It has a fixed width of 300 pixels for its contents. | ||
If you would like a site navigation bar for your site, you may create one easily using Markdown. | ||
|
||
- Start by creating a markdown file (`.md`) in `_markbind/navigation`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotta be consistent with Markdown (preferred) vs markdown.
docs/userGuide/contentAuthoring.md
Outdated
|
||
Note: | ||
- Only one navigation file can be specified in a page, and you must include its file extension. | ||
- There is no limit to the number of nested Dropdown menus but each menu's height is restricted to 1000 pixels. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comma before "but".
lib/Page.js
Outdated
+ '<div class="menu-top-bar"></div>\n' | ||
+ '<div class="menu-middle-bar"></div>\n' | ||
+ '<div class="menu-bottom-bar"></div>\n' | ||
+ '</div>\n</div>'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next line.
window.location = `${baseUrl}/${match.src.replace('.md', '.html')}`; | ||
const page = `${baseUrl}/${match.src.replace('.md', '.html')}`; | ||
const anchor = match.heading ? `#${match.heading.id}` : ''; | ||
window.location = `${page}${anchor}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes should disappear with a rebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you miss this?
asset/js/setup.js
Outdated
siteNavBtn.classList.toggle('morph'); | ||
document.getElementById('site-nav').classList.toggle('open'); | ||
document.getElementById('site-nav-btn-wrap').classList.toggle('open'); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Shall we reserve setup.js for Vue?
- Excessive space in
* site-nav-btn
. - Attach click handlers in JavaScript instead of having to do
// eslint-disable
.
asset/js/setup.js
Outdated
/** | ||
* Creates event listener for all dropdown-btns in page. | ||
*/ | ||
Array.prototype.forEach.call(document.getElementsByClassName('dropdown-btn'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Single-line comment since this isn't a function definition.
// Creates event listener for all dropdown-btns in page.
-
Why are we not doing the following?
document.getElementsByClassName('dropdown-btn').forEach((element) => {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we not doing the following?
This post explains it. I have found this updated solution to be a better alternative, using:
const dropdownBtnList = document.getElementByClassName('dropdown-btn');
for (let dropdownBtn of dropdownBtnList ) { ... }
Edit1: looks like ESlint doesn't like for loops. Will revert back to Array.prototype.forEach.call
as it seems most compatible with a wide range of browsers.
asset/css/markbind.css
Outdated
border-right: 1px solid lightgrey; | ||
display: inline-block; | ||
max-height: 80vh; | ||
font-size: 14pt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a huge section of styles. Is this from a source? Consider a separate stylesheet.
asset/js/setup.js
Outdated
dropdownContent.style.maxHeight = '0px'; | ||
} | ||
})); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
When dealing with huge blocks of code with lots of whitespace, prefer:
Array.prototype.forEach.call( document.getElementsByClassName('dropdown-btn'), element => element.addEventListener('click', () => { this.classList.toggle('dropdown-open'); const dropdownContent = this.nextElementSibling; if (dropdownContent.style.maxHeight === '0px' || !dropdownContent.style.maxHeight) { dropdownContent.style.transition = 'max-height 0.25s ease-in'; dropdownContent.style.maxHeight = '1000px'; } else { dropdownContent.style.transition = 'max-height 0.15s ease-out'; dropdownContent.style.maxHeight = '0px'; } }));
-
Extra empty line at end of file.
Update
TODO & under discussion
|
lib/Page.js
Outdated
const TITLE_PREFIX_SEPARATOR = ' - '; | ||
|
||
const DROPDOWN_BUTTON_ICON_HTML = '<i class="dropdown-btn-icon">\n' | ||
+ '<span class=\'glyphicon glyphicon-menu-down\' aria-hidden="true"></span>\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class can use "
instead of '
.
+ '</frontmatter>\n\n' | ||
+ '# Hello world\n' | ||
+ 'Welcome to your page generated with MarkBind.\n'; | ||
|
||
const SITE_NAV_DEFAULT = '<markdown>\n' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is wrapping the content with the <markdown>
tag absolutely necessary?
If it is so, might be a good thing to mention in the docs, because I didn't expect the tag to be necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tag was shown in the user guide example. will add additional emphasis.
lib/Page.js
Outdated
@@ -205,6 +288,7 @@ Page.prototype.generate = function (builtFiles) { | |||
this.collectFrontMatter(result); | |||
return this.removeFrontMatter(result); | |||
}) | |||
.then(result => this.insertSiteNav((result))) // Ensure this is before insertFooter |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was requested from @acjh for easy reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps explain it concisely (on its own line if need be) as the rationale is non-obvious?
docs/userGuide/contentAuthoring.md
Outdated
* [Dropdown link two](https://www.google.com/) | ||
* [==Third Link== :clipboard:](https://www.google.com/) | ||
* [Youtube](https://www.youtube.com/) <!-- Using a link as a Dropdown title will not render a Dropdown menu --> | ||
* [The answer to everything in the universe](https://www.youtube.com/watch?v=dQw4w9WgXcQ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other section has a YouTube URL as well. Let's standardize it by using the same URL:
http://www.youtube.com/watch?v=lJIrF4YjHfQ
a49dd74
to
3639797
Compare
Update
Discussion
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits
asset/css/site-nav.css
Outdated
|
||
/* Hamburger button morphing to close button */ | ||
|
||
.morph .menu-top-bar { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Morph is a name of a component in vue-strap as well, and is also used inside as a div class name. I think we should rename it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about .shift
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
@@ -250,8 +264,12 @@ Site.prototype.createPage = function (config) { | |||
path.join(this.siteAssetsDestPath, 'css', 'github.min.css')), | |||
markbind: path.relative(path.dirname(resultPath), | |||
path.join(this.siteAssetsDestPath, 'css', 'markbind.css')), | |||
siteNavCss: path.relative(path.dirname(resultPath), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Order was decided from file type (.css
) first, then name.
asset/css/site-nav.css
Outdated
display: block; | ||
margin-top: 20px; | ||
transition: 0.4s; | ||
top: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
top
should come before transition
.
* [Site Configuration]({{baseUrl}}/userGuide/siteConfiguration.html) | ||
* [Content Authoring]({{baseUrl}}/userGuide/contentAuthoring.html) | ||
* [Including Contents]({{baseUrl}}/userGuide/includingContents.html) | ||
* [Known Problems]({{baseUrl}}/userGuide/knownProblems.html); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the extra semi-colon at the back.
@@ -0,0 +1,12 @@ | |||
<markdown class="website-content"> | |||
* ## Sections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be more appropriate to say "User Guide" instead of "Sections" since this is now used everywhere.
docs/userGuide/contentAuthoring.md
Outdated
### Site Navigation | ||
|
||
A site navigation bar is a fixed menu on the left side of your page contents, that allows the viewer to navigate your site pages. | ||
It has a fixed width of 300 pixels for its contents. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The menu has a fixed width of 300 pixels.
docs/userGuide/contentAuthoring.md
Outdated
|
||
A site navigation bar is a fixed menu on the left side of your page contents, that allows the viewer to navigate your site pages. | ||
It has a fixed width of 300 pixels for its contents. | ||
If you would like a site navigation bar for your site, you may create one easily using Markdown. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...would like to have a site navigation...
docs/userGuide/contentAuthoring.md
Outdated
<li style="margin-top: 5px"><a href="https://www.google.com/"><mark>Third Link</mark> 📋</a></li> | ||
<a href="https://www.youtube.com/">Youtube</a> | ||
<ul style="list-style-type: none; margin-left:-1em"> | ||
<li style="margin-top: 5px"><a href="https://www.youtube.com/watch?v=dQw4w9WgXcQ">The answer to everything in the universe</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual rendered content need to match the original Markdown source.
docs/userGuide/contentAuthoring.md
Outdated
|
||
The site navigation bar has [responsive properties](https://www.w3schools.com/html/html_responsive.asp), and will collapse to a menu button when the screen width is smaller than 768 pixels. | ||
|
||
You are able to use [Markdown syntax](#supported-markdown-syntax), as well as [glyphicons](#glyphicons) to author your page navigation layout. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...page site navigation layout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly ok now, just something that suddenly popped up in my mind:
asset/js/site-nav.js
Outdated
/* eslint-disable no-undef */ | ||
|
||
// Add event listener for site-nav-btn to toggle itself and site navigation elements. | ||
document.getElementById('site-nav-btn') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
As mentioned, is site-nav.css (and site-nav.js) from a source or empirically determined?
lib/Page.js
Outdated
@@ -171,6 +226,34 @@ Page.prototype.removeFrontMatter = function (includedPage) { | |||
return $.html(); | |||
}; | |||
|
|||
Page.prototype.insertSiteNav = function (pageData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move after insertFooter
.
@@ -0,0 +1,21 @@ | |||
/* eslint-disable no-undef */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's specify environment instead:
/* eslint-env browser */
|
Apart from official credit, it's good to know where a non-obvious block of code or styles came from. Can you also update the PR description? In particular, the Changes table and the Demo gif. Thanks! |
5050be2
to
a7f040f
Compare
Update
|
Could you add that demo in a new page in test_site? Looks like a good stress test on site nav. |
How about I change the existing My demo page contents are just dummy values, don't think we need another page for it when we can test it fully on the main landing page 🔨 . |
The main site nav should be functional (appropriate links) though. |
You have a point. I am thinking of having this layout:
I feel it is appropriate as the main page is also testing several other elements at the same time. |
3633354
to
1a6dec1
Compare
Update
Discussion |
@@ -0,0 +1,5 @@ | |||
<footer> | |||
<div class="text-center"> | |||
{{timestamp}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be <small>[Generated by {{MarkBind}} on {{timestamp}}]</small>
1a6dec1
to
5a260e3
Compare
Update
|
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Documentation update
• [X] New feature
Resolves #220
What is the rationale for this request?
User would like a responsive site navigation bar to navigate site pages.
What changes did you make? (Give an overview)
site-nav.md
file duringinit
siteNav
attribute in a page's front matter. Formatting the markdown list to support dropdown menus after being rendered bymb.render(...)
nested dropdown menus
,site navigation styling
,morphing menu and close button
andresponsive site navigation media queries
dropdown-btn
to toggle next element (dropdown-container
), function forsite-nav-btn
to toggle.open
class for itself and all site-nav componentsIs there anything you'd like reviewers to focus on?
insertSiteNav
andformatSiteNav
coding stylesite-nav.md
site-nav.js
function location and code stylesite-nav.css
, layout and commentsTesting instructions:
markbind init
andmarkbind serve
. The default page contents and a site navigation bar should be loaded._markbind/navigation/site-nav.md
and reload server to see if changes are applied.siteNav
) in a folder (multiple nesting) and link it within the navigation file. The navigation bar should go to that page without any navigation bar rendered.