Update navbar usage and documentation - #790

Merged
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position
Mar 31, 2019
Merged

Update navbar usage and documentation#790
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position

Conversation

@Chng-Zhi-Xuan

@Chng-Zhi-XuanChng-Zhi-Xuan commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

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

• [x] Documentation update
• [x] Enhancement to an existing feature

Part of #733

Note:
This PR is branched from #733. Thus the commits are shared with it.
First unique commit is titled Asset: Update vue-strap.min.js.

What changes did you make? (Give an overview)

  • Added updated vue-strap.min.js from Remove position fixed classes from Navbar vue-strap#101
  • Updated navBars.mbdf to reflect new behaviour of navbars
  • Updated spacer-top class
  • Refactored usage of header.md within documentation and test_site to a header file
  • Added header content test within test_site

Is there anything you'd like reviewers to focus on?

  • Commit messages
  • Documentation changes

Testing instructions:

  • Browse documentation using Netlify Preview

marvinchin
marvinchin previously approved these changes Mar 25, 2019

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of nits 🍰

This class can be removed once the component's position type is changed.
*/
padding-top: 70px;
/* This class is to ensure adequate spacing of contents from the top of the page */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering - should this comment be placed outside the class definition (even though the comment was previously inside the class definition)? 🙂

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this section, how I formatted it was

/** * Main section description *//* sub section one *//* sub section two */
.some-class {
/* class description */
}

My intention was to separate sub section title comments from class descriptions

Comment threaddocs/userGuide/syntax/navBars.mbdf Outdated

<box type="warning">
<markdown>
Note: **Navbars** should be placed within a [header file]({{ baseUrl }}/userguide/tweakingthepagestructure.html#headers) to ensure correct positioning at the top of your page.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change "to ensure correct positioning" -> "to ensure that they are correctly positioned"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... to ensure that they are correctly positioned above page navigation and site navigation menus." ?

Added more explicit interaction description.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

"... to ensure that they are correctly positioned at the top of the page, above the page navigation and site navigation menus."

@marvinchin
marvinchin self-requested a review March 25, 2019 17:07
@marvinchin
marvinchin dismissed their stale reviewMarch 25, 2019 17:07

Wanted to clicked comment but hit approve instead :(

@luyangkennethluyangkenneth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

i.e. The more I scroll, the less of the navbar I see
Screenshot 2019-03-28 at 6 04 45 PM

@luyangkenneth

luyangkenneth commented Mar 28, 2019

Copy link
Copy Markdown
Contributor

Also, I'm not sure which specific PR introduced this change, but the "User Guide" heading in the site nav is indented when it shouldn't be.

@yamgent

Copy link
Copy Markdown
Member

As spoken, need to rebase this PR to get rid of the commits that originated from #733.

@yamgent

Copy link
Copy Markdown
Member

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

@damithc

Copy link
Copy Markdown
Contributor

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

As I understood, there are technical difficulties in making the navbar fixed after the recent changes to add support for page headers. Some (but not all) other similar sites such as Jekyll seem to have non-fixed nav bars too. So, I don't mind dropping the fixed navbar until we can bring back support for fixed navbars.

@Chng-Zhi-Xuan

Chng-Zhi-Xuan commented Mar 29, 2019

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Made minor documentation amendments, following review comments.

Discussion

Adopting the new holy grail layout require us to forgo position:fixed items. This is as we wanted to avoid using magic numbers to align the sticky side navigation menus if there was a position:fixed top navigation bar.

It also solves the issue of using anchor links and having a position:fixed top navbar block the heading.

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there's a merge conflict again.

@@ -0,0 +1,11 @@
<header>
<navbar placement="top" type="inverse">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placement="top" is redundant here.

The navbar component within the vue-strap repository has
been updated, thus we need to import the newly built
file into MarkBind.
Since header.md uses the navbar component, it is not
positioned correctly when used inline.
Let's shift header.md into the headers folder so that
it can be specified in the frontmatter for the pages
that require it.
The header.md contents will not be positioned correctly
unless it is specified as a header file.
Let's change usage of navbars within each page from
inline to specifying it within frontmatter.
Since the behaviour of the navbar component has changed,
let's update the documentation for it.
The workaround for the spacer-top class is not required
as navbars do not have position:fixed with the latest changes.
However, the contents of the menus are too close to the top of
the page after "sticking" to the top. Thus the padding has been
changed to 1rem to follow Bootstrap's navbar spacing conventions.
This is to test other content, aside from the navbar,
whether they will be placed correctly within the page.
@Chng-Zhi-Xuan

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Removed placement="top" from header.md in test_site

@yamgentyamgent added this to the v1.22.1 milestone Mar 30, 2019
@yamgent
yamgent merged commit ef167aa into MarkBind:masterMar 31, 2019
@Chng-Zhi-Xuan
Chng-Zhi-Xuan deleted the 733-navbar-position branch May 21, 2019 06:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Chng-Zhi-Xuan@luyangkenneth@yamgent@damithc@marvinchin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Update navbar usage and documentation - #790

Merged
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position
Mar 31, 2019
Merged

Update navbar usage and documentation#790
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position

Conversation

@Chng-Zhi-Xuan

@Chng-Zhi-XuanChng-Zhi-Xuan commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

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

• [x] Documentation update
• [x] Enhancement to an existing feature

Part of #733

Note:
This PR is branched from #733. Thus the commits are shared with it.
First unique commit is titled Asset: Update vue-strap.min.js.

What changes did you make? (Give an overview)

  • Added updated vue-strap.min.js from Remove position fixed classes from Navbar vue-strap#101
  • Updated navBars.mbdf to reflect new behaviour of navbars
  • Updated spacer-top class
  • Refactored usage of header.md within documentation and test_site to a header file
  • Added header content test within test_site

Is there anything you'd like reviewers to focus on?

  • Commit messages
  • Documentation changes

Testing instructions:

  • Browse documentation using Netlify Preview

marvinchin
marvinchin previously approved these changes Mar 25, 2019

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of nits 🍰

This class can be removed once the component's position type is changed.
*/
padding-top: 70px;
/* This class is to ensure adequate spacing of contents from the top of the page */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering - should this comment be placed outside the class definition (even though the comment was previously inside the class definition)? 🙂

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this section, how I formatted it was

/** * Main section description *//* sub section one *//* sub section two */
.some-class {
/* class description */
}

My intention was to separate sub section title comments from class descriptions

Comment threaddocs/userGuide/syntax/navBars.mbdf Outdated

<box type="warning">
<markdown>
Note: **Navbars** should be placed within a [header file]({{ baseUrl }}/userguide/tweakingthepagestructure.html#headers) to ensure correct positioning at the top of your page.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change "to ensure correct positioning" -> "to ensure that they are correctly positioned"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... to ensure that they are correctly positioned above page navigation and site navigation menus." ?

Added more explicit interaction description.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

"... to ensure that they are correctly positioned at the top of the page, above the page navigation and site navigation menus."

@marvinchin
marvinchin self-requested a review March 25, 2019 17:07
@marvinchin
marvinchin dismissed their stale reviewMarch 25, 2019 17:07

Wanted to clicked comment but hit approve instead :(

@luyangkennethluyangkenneth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

i.e. The more I scroll, the less of the navbar I see
Screenshot 2019-03-28 at 6 04 45 PM

@luyangkenneth

luyangkenneth commented Mar 28, 2019

Copy link
Copy Markdown
Contributor

Also, I'm not sure which specific PR introduced this change, but the "User Guide" heading in the site nav is indented when it shouldn't be.

@yamgent

Copy link
Copy Markdown
Member

As spoken, need to rebase this PR to get rid of the commits that originated from #733.

@yamgent

Copy link
Copy Markdown
Member

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

@damithc

Copy link
Copy Markdown
Contributor

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

As I understood, there are technical difficulties in making the navbar fixed after the recent changes to add support for page headers. Some (but not all) other similar sites such as Jekyll seem to have non-fixed nav bars too. So, I don't mind dropping the fixed navbar until we can bring back support for fixed navbars.

@Chng-Zhi-Xuan

Chng-Zhi-Xuan commented Mar 29, 2019

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Made minor documentation amendments, following review comments.

Discussion

Adopting the new holy grail layout require us to forgo position:fixed items. This is as we wanted to avoid using magic numbers to align the sticky side navigation menus if there was a position:fixed top navigation bar.

It also solves the issue of using anchor links and having a position:fixed top navbar block the heading.

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there's a merge conflict again.

@@ -0,0 +1,11 @@
<header>
<navbar placement="top" type="inverse">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placement="top" is redundant here.

The navbar component within the vue-strap repository has
been updated, thus we need to import the newly built
file into MarkBind.
Since header.md uses the navbar component, it is not
positioned correctly when used inline.
Let's shift header.md into the headers folder so that
it can be specified in the frontmatter for the pages
that require it.
The header.md contents will not be positioned correctly
unless it is specified as a header file.
Let's change usage of navbars within each page from
inline to specifying it within frontmatter.
Since the behaviour of the navbar component has changed,
let's update the documentation for it.
The workaround for the spacer-top class is not required
as navbars do not have position:fixed with the latest changes.
However, the contents of the menus are too close to the top of
the page after "sticking" to the top. Thus the padding has been
changed to 1rem to follow Bootstrap's navbar spacing conventions.
This is to test other content, aside from the navbar,
whether they will be placed correctly within the page.
@Chng-Zhi-Xuan

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Removed placement="top" from header.md in test_site

@yamgentyamgent added this to the v1.22.1 milestone Mar 30, 2019
@yamgent
yamgent merged commit ef167aa into MarkBind:masterMar 31, 2019
@Chng-Zhi-Xuan
Chng-Zhi-Xuan deleted the 733-navbar-position branch May 21, 2019 06:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Chng-Zhi-Xuan@luyangkenneth@yamgent@damithc@marvinchin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Update navbar usage and documentation - #790

Merged
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position
Mar 31, 2019
Merged

Update navbar usage and documentation#790
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position

Conversation

@Chng-Zhi-Xuan

@Chng-Zhi-XuanChng-Zhi-Xuan commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

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

• [x] Documentation update
• [x] Enhancement to an existing feature

Part of #733

Note:
This PR is branched from #733. Thus the commits are shared with it.
First unique commit is titled Asset: Update vue-strap.min.js.

What changes did you make? (Give an overview)

  • Added updated vue-strap.min.js from Remove position fixed classes from Navbar vue-strap#101
  • Updated navBars.mbdf to reflect new behaviour of navbars
  • Updated spacer-top class
  • Refactored usage of header.md within documentation and test_site to a header file
  • Added header content test within test_site

Is there anything you'd like reviewers to focus on?

  • Commit messages
  • Documentation changes

Testing instructions:

  • Browse documentation using Netlify Preview

marvinchin
marvinchin previously approved these changes Mar 25, 2019

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of nits 🍰

This class can be removed once the component's position type is changed.
*/
padding-top: 70px;
/* This class is to ensure adequate spacing of contents from the top of the page */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering - should this comment be placed outside the class definition (even though the comment was previously inside the class definition)? 🙂

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this section, how I formatted it was

/** * Main section description *//* sub section one *//* sub section two */
.some-class {
/* class description */
}

My intention was to separate sub section title comments from class descriptions

Comment threaddocs/userGuide/syntax/navBars.mbdf Outdated

<box type="warning">
<markdown>
Note: **Navbars** should be placed within a [header file]({{ baseUrl }}/userguide/tweakingthepagestructure.html#headers) to ensure correct positioning at the top of your page.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change "to ensure correct positioning" -> "to ensure that they are correctly positioned"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... to ensure that they are correctly positioned above page navigation and site navigation menus." ?

Added more explicit interaction description.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

"... to ensure that they are correctly positioned at the top of the page, above the page navigation and site navigation menus."

@marvinchin
marvinchin self-requested a review March 25, 2019 17:07
@marvinchin
marvinchin dismissed their stale reviewMarch 25, 2019 17:07

Wanted to clicked comment but hit approve instead :(

@luyangkennethluyangkenneth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

i.e. The more I scroll, the less of the navbar I see
Screenshot 2019-03-28 at 6 04 45 PM

@luyangkenneth

luyangkenneth commented Mar 28, 2019

Copy link
Copy Markdown
Contributor

Also, I'm not sure which specific PR introduced this change, but the "User Guide" heading in the site nav is indented when it shouldn't be.

@yamgent

Copy link
Copy Markdown
Member

As spoken, need to rebase this PR to get rid of the commits that originated from #733.

@yamgent

Copy link
Copy Markdown
Member

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

@damithc

Copy link
Copy Markdown
Contributor

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

As I understood, there are technical difficulties in making the navbar fixed after the recent changes to add support for page headers. Some (but not all) other similar sites such as Jekyll seem to have non-fixed nav bars too. So, I don't mind dropping the fixed navbar until we can bring back support for fixed navbars.

@Chng-Zhi-Xuan

Chng-Zhi-Xuan commented Mar 29, 2019

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Made minor documentation amendments, following review comments.

Discussion

Adopting the new holy grail layout require us to forgo position:fixed items. This is as we wanted to avoid using magic numbers to align the sticky side navigation menus if there was a position:fixed top navigation bar.

It also solves the issue of using anchor links and having a position:fixed top navbar block the heading.

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there's a merge conflict again.

@@ -0,0 +1,11 @@
<header>
<navbar placement="top" type="inverse">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placement="top" is redundant here.

The navbar component within the vue-strap repository has
been updated, thus we need to import the newly built
file into MarkBind.
Since header.md uses the navbar component, it is not
positioned correctly when used inline.
Let's shift header.md into the headers folder so that
it can be specified in the frontmatter for the pages
that require it.
The header.md contents will not be positioned correctly
unless it is specified as a header file.
Let's change usage of navbars within each page from
inline to specifying it within frontmatter.
Since the behaviour of the navbar component has changed,
let's update the documentation for it.
The workaround for the spacer-top class is not required
as navbars do not have position:fixed with the latest changes.
However, the contents of the menus are too close to the top of
the page after "sticking" to the top. Thus the padding has been
changed to 1rem to follow Bootstrap's navbar spacing conventions.
This is to test other content, aside from the navbar,
whether they will be placed correctly within the page.
@Chng-Zhi-Xuan

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Removed placement="top" from header.md in test_site

@yamgentyamgent added this to the v1.22.1 milestone Mar 30, 2019
@yamgent
yamgent merged commit ef167aa into MarkBind:masterMar 31, 2019
@Chng-Zhi-Xuan
Chng-Zhi-Xuan deleted the 733-navbar-position branch May 21, 2019 06:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Chng-Zhi-Xuan@luyangkenneth@yamgent@damithc@marvinchin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Update navbar usage and documentation - #790

Merged
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position
Mar 31, 2019
Merged

Update navbar usage and documentation#790
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position

Conversation

@Chng-Zhi-Xuan

@Chng-Zhi-XuanChng-Zhi-Xuan commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

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

• [x] Documentation update
• [x] Enhancement to an existing feature

Part of #733

Note:
This PR is branched from #733. Thus the commits are shared with it.
First unique commit is titled Asset: Update vue-strap.min.js.

What changes did you make? (Give an overview)

  • Added updated vue-strap.min.js from Remove position fixed classes from Navbar vue-strap#101
  • Updated navBars.mbdf to reflect new behaviour of navbars
  • Updated spacer-top class
  • Refactored usage of header.md within documentation and test_site to a header file
  • Added header content test within test_site

Is there anything you'd like reviewers to focus on?

  • Commit messages
  • Documentation changes

Testing instructions:

  • Browse documentation using Netlify Preview

marvinchin
marvinchin previously approved these changes Mar 25, 2019

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of nits 🍰

This class can be removed once the component's position type is changed.
*/
padding-top: 70px;
/* This class is to ensure adequate spacing of contents from the top of the page */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering - should this comment be placed outside the class definition (even though the comment was previously inside the class definition)? 🙂

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this section, how I formatted it was

/** * Main section description *//* sub section one *//* sub section two */
.some-class {
/* class description */
}

My intention was to separate sub section title comments from class descriptions

Comment threaddocs/userGuide/syntax/navBars.mbdf Outdated

<box type="warning">
<markdown>
Note: **Navbars** should be placed within a [header file]({{ baseUrl }}/userguide/tweakingthepagestructure.html#headers) to ensure correct positioning at the top of your page.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change "to ensure correct positioning" -> "to ensure that they are correctly positioned"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... to ensure that they are correctly positioned above page navigation and site navigation menus." ?

Added more explicit interaction description.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

"... to ensure that they are correctly positioned at the top of the page, above the page navigation and site navigation menus."

@marvinchin
marvinchin self-requested a review March 25, 2019 17:07
@marvinchin
marvinchin dismissed their stale reviewMarch 25, 2019 17:07

Wanted to clicked comment but hit approve instead :(

@luyangkennethluyangkenneth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

i.e. The more I scroll, the less of the navbar I see
Screenshot 2019-03-28 at 6 04 45 PM

@luyangkenneth

luyangkenneth commented Mar 28, 2019

Copy link
Copy Markdown
Contributor

Also, I'm not sure which specific PR introduced this change, but the "User Guide" heading in the site nav is indented when it shouldn't be.

@yamgent

Copy link
Copy Markdown
Member

As spoken, need to rebase this PR to get rid of the commits that originated from #733.

@yamgent

Copy link
Copy Markdown
Member

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

@damithc

Copy link
Copy Markdown
Contributor

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

As I understood, there are technical difficulties in making the navbar fixed after the recent changes to add support for page headers. Some (but not all) other similar sites such as Jekyll seem to have non-fixed nav bars too. So, I don't mind dropping the fixed navbar until we can bring back support for fixed navbars.

@Chng-Zhi-Xuan

Chng-Zhi-Xuan commented Mar 29, 2019

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Made minor documentation amendments, following review comments.

Discussion

Adopting the new holy grail layout require us to forgo position:fixed items. This is as we wanted to avoid using magic numbers to align the sticky side navigation menus if there was a position:fixed top navigation bar.

It also solves the issue of using anchor links and having a position:fixed top navbar block the heading.

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there's a merge conflict again.

@@ -0,0 +1,11 @@
<header>
<navbar placement="top" type="inverse">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placement="top" is redundant here.

The navbar component within the vue-strap repository has
been updated, thus we need to import the newly built
file into MarkBind.
Since header.md uses the navbar component, it is not
positioned correctly when used inline.
Let's shift header.md into the headers folder so that
it can be specified in the frontmatter for the pages
that require it.
The header.md contents will not be positioned correctly
unless it is specified as a header file.
Let's change usage of navbars within each page from
inline to specifying it within frontmatter.
Since the behaviour of the navbar component has changed,
let's update the documentation for it.
The workaround for the spacer-top class is not required
as navbars do not have position:fixed with the latest changes.
However, the contents of the menus are too close to the top of
the page after "sticking" to the top. Thus the padding has been
changed to 1rem to follow Bootstrap's navbar spacing conventions.
This is to test other content, aside from the navbar,
whether they will be placed correctly within the page.
@Chng-Zhi-Xuan

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Removed placement="top" from header.md in test_site

@yamgentyamgent added this to the v1.22.1 milestone Mar 30, 2019
@yamgent
yamgent merged commit ef167aa into MarkBind:masterMar 31, 2019
@Chng-Zhi-Xuan
Chng-Zhi-Xuan deleted the 733-navbar-position branch May 21, 2019 06:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Chng-Zhi-Xuan@luyangkenneth@yamgent@damithc@marvinchin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Update navbar usage and documentation - #790

Merged
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position
Mar 31, 2019
Merged

Update navbar usage and documentation#790
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position

Conversation

@Chng-Zhi-Xuan

@Chng-Zhi-XuanChng-Zhi-Xuan commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

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

• [x] Documentation update
• [x] Enhancement to an existing feature

Part of #733

Note:
This PR is branched from #733. Thus the commits are shared with it.
First unique commit is titled Asset: Update vue-strap.min.js.

What changes did you make? (Give an overview)

  • Added updated vue-strap.min.js from Remove position fixed classes from Navbar vue-strap#101
  • Updated navBars.mbdf to reflect new behaviour of navbars
  • Updated spacer-top class
  • Refactored usage of header.md within documentation and test_site to a header file
  • Added header content test within test_site

Is there anything you'd like reviewers to focus on?

  • Commit messages
  • Documentation changes

Testing instructions:

  • Browse documentation using Netlify Preview

marvinchin
marvinchin previously approved these changes Mar 25, 2019

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of nits 🍰

This class can be removed once the component's position type is changed.
*/
padding-top: 70px;
/* This class is to ensure adequate spacing of contents from the top of the page */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering - should this comment be placed outside the class definition (even though the comment was previously inside the class definition)? 🙂

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this section, how I formatted it was

/** * Main section description *//* sub section one *//* sub section two */
.some-class {
/* class description */
}

My intention was to separate sub section title comments from class descriptions

Comment threaddocs/userGuide/syntax/navBars.mbdf Outdated

<box type="warning">
<markdown>
Note: **Navbars** should be placed within a [header file]({{ baseUrl }}/userguide/tweakingthepagestructure.html#headers) to ensure correct positioning at the top of your page.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change "to ensure correct positioning" -> "to ensure that they are correctly positioned"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... to ensure that they are correctly positioned above page navigation and site navigation menus." ?

Added more explicit interaction description.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

"... to ensure that they are correctly positioned at the top of the page, above the page navigation and site navigation menus."

@marvinchin
marvinchin self-requested a review March 25, 2019 17:07
@marvinchin
marvinchin dismissed their stale reviewMarch 25, 2019 17:07

Wanted to clicked comment but hit approve instead :(

@luyangkennethluyangkenneth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

i.e. The more I scroll, the less of the navbar I see
Screenshot 2019-03-28 at 6 04 45 PM

@luyangkenneth

luyangkenneth commented Mar 28, 2019

Copy link
Copy Markdown
Contributor

Also, I'm not sure which specific PR introduced this change, but the "User Guide" heading in the site nav is indented when it shouldn't be.

@yamgent

Copy link
Copy Markdown
Member

As spoken, need to rebase this PR to get rid of the commits that originated from #733.

@yamgent

Copy link
Copy Markdown
Member

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

@damithc

Copy link
Copy Markdown
Contributor

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

As I understood, there are technical difficulties in making the navbar fixed after the recent changes to add support for page headers. Some (but not all) other similar sites such as Jekyll seem to have non-fixed nav bars too. So, I don't mind dropping the fixed navbar until we can bring back support for fixed navbars.

@Chng-Zhi-Xuan

Chng-Zhi-Xuan commented Mar 29, 2019

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Made minor documentation amendments, following review comments.

Discussion

Adopting the new holy grail layout require us to forgo position:fixed items. This is as we wanted to avoid using magic numbers to align the sticky side navigation menus if there was a position:fixed top navigation bar.

It also solves the issue of using anchor links and having a position:fixed top navbar block the heading.

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there's a merge conflict again.

@@ -0,0 +1,11 @@
<header>
<navbar placement="top" type="inverse">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placement="top" is redundant here.

The navbar component within the vue-strap repository has
been updated, thus we need to import the newly built
file into MarkBind.
Since header.md uses the navbar component, it is not
positioned correctly when used inline.
Let's shift header.md into the headers folder so that
it can be specified in the frontmatter for the pages
that require it.
The header.md contents will not be positioned correctly
unless it is specified as a header file.
Let's change usage of navbars within each page from
inline to specifying it within frontmatter.
Since the behaviour of the navbar component has changed,
let's update the documentation for it.
The workaround for the spacer-top class is not required
as navbars do not have position:fixed with the latest changes.
However, the contents of the menus are too close to the top of
the page after "sticking" to the top. Thus the padding has been
changed to 1rem to follow Bootstrap's navbar spacing conventions.
This is to test other content, aside from the navbar,
whether they will be placed correctly within the page.
@Chng-Zhi-Xuan

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Removed placement="top" from header.md in test_site

@yamgentyamgent added this to the v1.22.1 milestone Mar 30, 2019
@yamgent
yamgent merged commit ef167aa into MarkBind:masterMar 31, 2019
@Chng-Zhi-Xuan
Chng-Zhi-Xuan deleted the 733-navbar-position branch May 21, 2019 06:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Chng-Zhi-Xuan@luyangkenneth@yamgent@damithc@marvinchin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Update navbar usage and documentation - #790

Merged
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position
Mar 31, 2019
Merged

Update navbar usage and documentation#790
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position

Conversation

@Chng-Zhi-Xuan

@Chng-Zhi-XuanChng-Zhi-Xuan commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

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

• [x] Documentation update
• [x] Enhancement to an existing feature

Part of #733

Note:
This PR is branched from #733. Thus the commits are shared with it.
First unique commit is titled Asset: Update vue-strap.min.js.

What changes did you make? (Give an overview)

  • Added updated vue-strap.min.js from Remove position fixed classes from Navbar vue-strap#101
  • Updated navBars.mbdf to reflect new behaviour of navbars
  • Updated spacer-top class
  • Refactored usage of header.md within documentation and test_site to a header file
  • Added header content test within test_site

Is there anything you'd like reviewers to focus on?

  • Commit messages
  • Documentation changes

Testing instructions:

  • Browse documentation using Netlify Preview

marvinchin
marvinchin previously approved these changes Mar 25, 2019

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of nits 🍰

This class can be removed once the component's position type is changed.
*/
padding-top: 70px;
/* This class is to ensure adequate spacing of contents from the top of the page */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering - should this comment be placed outside the class definition (even though the comment was previously inside the class definition)? 🙂

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this section, how I formatted it was

/** * Main section description *//* sub section one *//* sub section two */
.some-class {
/* class description */
}

My intention was to separate sub section title comments from class descriptions

Comment threaddocs/userGuide/syntax/navBars.mbdf Outdated

<box type="warning">
<markdown>
Note: **Navbars** should be placed within a [header file]({{ baseUrl }}/userguide/tweakingthepagestructure.html#headers) to ensure correct positioning at the top of your page.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change "to ensure correct positioning" -> "to ensure that they are correctly positioned"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... to ensure that they are correctly positioned above page navigation and site navigation menus." ?

Added more explicit interaction description.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

"... to ensure that they are correctly positioned at the top of the page, above the page navigation and site navigation menus."

@marvinchin
marvinchin self-requested a review March 25, 2019 17:07
@marvinchin
marvinchin dismissed their stale reviewMarch 25, 2019 17:07

Wanted to clicked comment but hit approve instead :(

@luyangkennethluyangkenneth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

i.e. The more I scroll, the less of the navbar I see
Screenshot 2019-03-28 at 6 04 45 PM

@luyangkenneth

luyangkenneth commented Mar 28, 2019

Copy link
Copy Markdown
Contributor

Also, I'm not sure which specific PR introduced this change, but the "User Guide" heading in the site nav is indented when it shouldn't be.

@yamgent

Copy link
Copy Markdown
Member

As spoken, need to rebase this PR to get rid of the commits that originated from #733.

@yamgent

Copy link
Copy Markdown
Member

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

@damithc

Copy link
Copy Markdown
Contributor

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

As I understood, there are technical difficulties in making the navbar fixed after the recent changes to add support for page headers. Some (but not all) other similar sites such as Jekyll seem to have non-fixed nav bars too. So, I don't mind dropping the fixed navbar until we can bring back support for fixed navbars.

@Chng-Zhi-Xuan

Chng-Zhi-Xuan commented Mar 29, 2019

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Made minor documentation amendments, following review comments.

Discussion

Adopting the new holy grail layout require us to forgo position:fixed items. This is as we wanted to avoid using magic numbers to align the sticky side navigation menus if there was a position:fixed top navigation bar.

It also solves the issue of using anchor links and having a position:fixed top navbar block the heading.

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there's a merge conflict again.

@@ -0,0 +1,11 @@
<header>
<navbar placement="top" type="inverse">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placement="top" is redundant here.

The navbar component within the vue-strap repository has
been updated, thus we need to import the newly built
file into MarkBind.
Since header.md uses the navbar component, it is not
positioned correctly when used inline.
Let's shift header.md into the headers folder so that
it can be specified in the frontmatter for the pages
that require it.
The header.md contents will not be positioned correctly
unless it is specified as a header file.
Let's change usage of navbars within each page from
inline to specifying it within frontmatter.
Since the behaviour of the navbar component has changed,
let's update the documentation for it.
The workaround for the spacer-top class is not required
as navbars do not have position:fixed with the latest changes.
However, the contents of the menus are too close to the top of
the page after "sticking" to the top. Thus the padding has been
changed to 1rem to follow Bootstrap's navbar spacing conventions.
This is to test other content, aside from the navbar,
whether they will be placed correctly within the page.
@Chng-Zhi-Xuan

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Removed placement="top" from header.md in test_site

@yamgentyamgent added this to the v1.22.1 milestone Mar 30, 2019
@yamgent
yamgent merged commit ef167aa into MarkBind:masterMar 31, 2019
@Chng-Zhi-Xuan
Chng-Zhi-Xuan deleted the 733-navbar-position branch May 21, 2019 06:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Chng-Zhi-Xuan@luyangkenneth@yamgent@damithc@marvinchin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Update navbar usage and documentation - #790

Merged
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position
Mar 31, 2019
Merged

Update navbar usage and documentation#790
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position

Conversation

@Chng-Zhi-Xuan

@Chng-Zhi-XuanChng-Zhi-Xuan commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

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

• [x] Documentation update
• [x] Enhancement to an existing feature

Part of #733

Note:
This PR is branched from #733. Thus the commits are shared with it.
First unique commit is titled Asset: Update vue-strap.min.js.

What changes did you make? (Give an overview)

  • Added updated vue-strap.min.js from Remove position fixed classes from Navbar vue-strap#101
  • Updated navBars.mbdf to reflect new behaviour of navbars
  • Updated spacer-top class
  • Refactored usage of header.md within documentation and test_site to a header file
  • Added header content test within test_site

Is there anything you'd like reviewers to focus on?

  • Commit messages
  • Documentation changes

Testing instructions:

  • Browse documentation using Netlify Preview

marvinchin
marvinchin previously approved these changes Mar 25, 2019

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of nits 🍰

This class can be removed once the component's position type is changed.
*/
padding-top: 70px;
/* This class is to ensure adequate spacing of contents from the top of the page */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering - should this comment be placed outside the class definition (even though the comment was previously inside the class definition)? 🙂

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this section, how I formatted it was

/** * Main section description *//* sub section one *//* sub section two */
.some-class {
/* class description */
}

My intention was to separate sub section title comments from class descriptions

Comment threaddocs/userGuide/syntax/navBars.mbdf Outdated

<box type="warning">
<markdown>
Note: **Navbars** should be placed within a [header file]({{ baseUrl }}/userguide/tweakingthepagestructure.html#headers) to ensure correct positioning at the top of your page.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change "to ensure correct positioning" -> "to ensure that they are correctly positioned"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... to ensure that they are correctly positioned above page navigation and site navigation menus." ?

Added more explicit interaction description.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

"... to ensure that they are correctly positioned at the top of the page, above the page navigation and site navigation menus."

@marvinchin
marvinchin self-requested a review March 25, 2019 17:07
@marvinchin
marvinchin dismissed their stale reviewMarch 25, 2019 17:07

Wanted to clicked comment but hit approve instead :(

@luyangkennethluyangkenneth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

i.e. The more I scroll, the less of the navbar I see
Screenshot 2019-03-28 at 6 04 45 PM

@luyangkenneth

luyangkenneth commented Mar 28, 2019

Copy link
Copy Markdown
Contributor

Also, I'm not sure which specific PR introduced this change, but the "User Guide" heading in the site nav is indented when it shouldn't be.

@yamgent

Copy link
Copy Markdown
Member

As spoken, need to rebase this PR to get rid of the commits that originated from #733.

@yamgent

Copy link
Copy Markdown
Member

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

@damithc

Copy link
Copy Markdown
Contributor

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

As I understood, there are technical difficulties in making the navbar fixed after the recent changes to add support for page headers. Some (but not all) other similar sites such as Jekyll seem to have non-fixed nav bars too. So, I don't mind dropping the fixed navbar until we can bring back support for fixed navbars.

@Chng-Zhi-Xuan

Chng-Zhi-Xuan commented Mar 29, 2019

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Made minor documentation amendments, following review comments.

Discussion

Adopting the new holy grail layout require us to forgo position:fixed items. This is as we wanted to avoid using magic numbers to align the sticky side navigation menus if there was a position:fixed top navigation bar.

It also solves the issue of using anchor links and having a position:fixed top navbar block the heading.

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there's a merge conflict again.

@@ -0,0 +1,11 @@
<header>
<navbar placement="top" type="inverse">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placement="top" is redundant here.

The navbar component within the vue-strap repository has
been updated, thus we need to import the newly built
file into MarkBind.
Since header.md uses the navbar component, it is not
positioned correctly when used inline.
Let's shift header.md into the headers folder so that
it can be specified in the frontmatter for the pages
that require it.
The header.md contents will not be positioned correctly
unless it is specified as a header file.
Let's change usage of navbars within each page from
inline to specifying it within frontmatter.
Since the behaviour of the navbar component has changed,
let's update the documentation for it.
The workaround for the spacer-top class is not required
as navbars do not have position:fixed with the latest changes.
However, the contents of the menus are too close to the top of
the page after "sticking" to the top. Thus the padding has been
changed to 1rem to follow Bootstrap's navbar spacing conventions.
This is to test other content, aside from the navbar,
whether they will be placed correctly within the page.
@Chng-Zhi-Xuan

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Removed placement="top" from header.md in test_site

@yamgentyamgent added this to the v1.22.1 milestone Mar 30, 2019
@yamgent
yamgent merged commit ef167aa into MarkBind:masterMar 31, 2019
@Chng-Zhi-Xuan
Chng-Zhi-Xuan deleted the 733-navbar-position branch May 21, 2019 06:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Chng-Zhi-Xuan@luyangkenneth@yamgent@damithc@marvinchin
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Update navbar usage and documentation - #790

Merged
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position
Mar 31, 2019
Merged

Update navbar usage and documentation#790
yamgent merged 8 commits into
MarkBind:masterfrom
Chng-Zhi-Xuan:733-navbar-position

Conversation

@Chng-Zhi-Xuan

@Chng-Zhi-XuanChng-Zhi-Xuan commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

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

• [x] Documentation update
• [x] Enhancement to an existing feature

Part of #733

Note:
This PR is branched from #733. Thus the commits are shared with it.
First unique commit is titled Asset: Update vue-strap.min.js.

What changes did you make? (Give an overview)

  • Added updated vue-strap.min.js from Remove position fixed classes from Navbar vue-strap#101
  • Updated navBars.mbdf to reflect new behaviour of navbars
  • Updated spacer-top class
  • Refactored usage of header.md within documentation and test_site to a header file
  • Added header content test within test_site

Is there anything you'd like reviewers to focus on?

  • Commit messages
  • Documentation changes

Testing instructions:

  • Browse documentation using Netlify Preview

marvinchin
marvinchin previously approved these changes Mar 25, 2019

@marvinchinmarvinchin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a couple of nits 🍰

This class can be removed once the component's position type is changed.
*/
padding-top: 70px;
/* This class is to ensure adequate spacing of contents from the top of the page */

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering - should this comment be placed outside the class definition (even though the comment was previously inside the class definition)? 🙂

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this section, how I formatted it was

/** * Main section description *//* sub section one *//* sub section two */
.some-class {
/* class description */
}

My intention was to separate sub section title comments from class descriptions

Comment threaddocs/userGuide/syntax/navBars.mbdf Outdated

<box type="warning">
<markdown>
Note: **Navbars** should be placed within a [header file]({{ baseUrl }}/userguide/tweakingthepagestructure.html#headers) to ensure correct positioning at the top of your page.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change "to ensure correct positioning" -> "to ensure that they are correctly positioned"?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"... to ensure that they are correctly positioned above page navigation and site navigation menus." ?

Added more explicit interaction description.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

"... to ensure that they are correctly positioned at the top of the page, above the page navigation and site navigation menus."

@marvinchin
marvinchin self-requested a review March 25, 2019 17:07
@marvinchin
marvinchin dismissed their stale reviewMarch 25, 2019 17:07

Wanted to clicked comment but hit approve instead :(

@luyangkennethluyangkenneth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

i.e. The more I scroll, the less of the navbar I see
Screenshot 2019-03-28 at 6 04 45 PM

@luyangkenneth

luyangkenneth commented Mar 28, 2019

Copy link
Copy Markdown
Contributor

Also, I'm not sure which specific PR introduced this change, but the "User Guide" heading in the site nav is indented when it shouldn't be.

@yamgent

Copy link
Copy Markdown
Member

As spoken, need to rebase this PR to get rid of the commits that originated from #733.

@yamgent

Copy link
Copy Markdown
Member

Seems like the navbar is no longer fixed to the top of the page. Is this intentional?

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

@damithc

Copy link
Copy Markdown
Contributor

@luyangkenneth As discussed with Prof, yes this is intentional, it follows the style of other websites such as Jekyll's home page.

As I understood, there are technical difficulties in making the navbar fixed after the recent changes to add support for page headers. Some (but not all) other similar sites such as Jekyll seem to have non-fixed nav bars too. So, I don't mind dropping the fixed navbar until we can bring back support for fixed navbars.

@Chng-Zhi-Xuan

Chng-Zhi-Xuan commented Mar 29, 2019

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Made minor documentation amendments, following review comments.

Discussion

Adopting the new holy grail layout require us to forgo position:fixed items. This is as we wanted to avoid using magic numbers to align the sticky side navigation menus if there was a position:fixed top navigation bar.

It also solves the issue of using anchor links and having a position:fixed top navbar block the heading.

@yamgentyamgent left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there's a merge conflict again.

@@ -0,0 +1,11 @@
<header>
<navbar placement="top" type="inverse">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

placement="top" is redundant here.

The navbar component within the vue-strap repository has
been updated, thus we need to import the newly built
file into MarkBind.
Since header.md uses the navbar component, it is not
positioned correctly when used inline.
Let's shift header.md into the headers folder so that
it can be specified in the frontmatter for the pages
that require it.
The header.md contents will not be positioned correctly
unless it is specified as a header file.
Let's change usage of navbars within each page from
inline to specifying it within frontmatter.
Since the behaviour of the navbar component has changed,
let's update the documentation for it.
The workaround for the spacer-top class is not required
as navbars do not have position:fixed with the latest changes.
However, the contents of the menus are too close to the top of
the page after "sticking" to the top. Thus the padding has been
changed to 1rem to follow Bootstrap's navbar spacing conventions.
This is to test other content, aside from the navbar,
whether they will be placed correctly within the page.
@Chng-Zhi-Xuan

Copy link
Copy Markdown
ContributorAuthor

Update

  • Rebase to latest master
  • Removed placement="top" from header.md in test_site

@yamgentyamgent added this to the v1.22.1 milestone Mar 30, 2019
@yamgent
yamgent merged commit ef167aa into MarkBind:masterMar 31, 2019
@Chng-Zhi-Xuan
Chng-Zhi-Xuan deleted the 733-navbar-position branch May 21, 2019 06:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@Chng-Zhi-Xuan@luyangkenneth@yamgent@damithc@marvinchin