Skip to content
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 a 'page top' container #636

Closed
damithc opened this issue Jan 27, 2019 · 9 comments · Fixed by #733
Closed

Support a 'page top' container #636

damithc opened this issue Jan 27, 2019 · 9 comments · Fixed by #733

Comments

@damithc
Copy link
Contributor

damithc commented Jan 27, 2019

Should we support a generic container for placing some content at the top of the page in a way that SiteNav and PageNav play along? i.e., SiteNav and PageNav should appear below this container.

e.g.,

<pagetop sticky flush>
... content
</pagetop>

Example user case: if the user wants to use Bootstrap navigation menus instead of our own navbar

@damithc damithc added s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding a-Syntax labels Jan 27, 2019
@damithc
Copy link
Contributor Author

damithc commented Jan 27, 2019

Somewhat related to #549

@damithc
Copy link
Contributor Author

damithc commented Jan 27, 2019

@Chng-Zhi-Xuan
To give a more concrete example, I would like to add SiteNav and PageNav to se-edu/learningresources as at commit se-edu/learningresources@a64c6c8 which contains a custom TopNav (not MarkBind navbar)

@Chng-Zhi-Xuan
Copy link
Contributor

Chng-Zhi-Xuan commented Jan 29, 2019

Sorry for the late reply, I will need to refresh myself regarding the behaviour of site-nav and page-nav interaction with navbar. I am currently aiming to construct a class to mimic a fixed-top behaviour while having site-nav and page-nav align properly.

The class then can just be added to markbind.css and used easily.

@pyokagan
Copy link

pyokagan commented Feb 3, 2019

I am currently aiming to construct a class to mimic a fixed-top behaviour while having site-nav and page-nav align properly.

I'm planning to use position:sticky, where the container the navbar is in also matters as well.

The major issue I have is that the generated HTML by MarkBind doesn't fit the visual layout of the page as well. It goes like this:

<body>
<div id="app">
  <!-- page-nav is the first thing that search engines will see rather than the site header, may affect SEO -->
  <nav id="page-nav">
    ...
  </nav>
  <div id="flex-body"> <!-- No idea what the point of flex-body is here -->
    <div id="site-nav">
      ...
    </div>
    <div id="page-content">
      { THE TOP NAVBAR IS HERE }
      ...
    </div>
  </div>
  <!-- There is a <footer> but no <header> :thinking emoji: -->
  <footer>
    ...
  </footer>
</div>
</body>

As we can see, the "top navbar" is nested inside the page content, which makes it hard to write CSS that positions it independently of the page content (position:fixed is just a hack). No idea why the page-nav is not inside the same container as site-nav, it makes it harder to write CSS that positions them based on their common container.

Anyway, if there is a new <pagetop> element, I'd like its contents to be right after the opening <body><div id="app"> tag:

<body>
<div id="app">
  { INSERT PAGE TOP HERE}
  <nav id="page-nav">
    ...
  </nav>
  <div id="flex-body">
    <div id="site-nav">
      ...
    </div>
    <div id="page-content">
      ...
    </div>
  </div>
  <footer>
    ...
  </footer>
</div>
</body>

Preferably, the source location of the page-nav and site-nav should be fixed as well, such that they share the same nesting level, or page-nav has a greater nesting level than site-nav (mirroring the fact that site-nav provides a "higher level" navigation compared to page-nav):

<body>
<div id="app">
  <header>
    { PAGE TOP }
  </header>
  <div id="flex-body">
    <div id="site-nav">
      ...
    </div>
    <div id="page-nav">
      ...
    </div>
    <div id="page-content">
      ...
    </div>
  </div>
  <footer>
    ...
  </footer>
</div>
</body>

@pyokagan
Copy link

pyokagan commented Feb 8, 2019

The behavior of the page nav in https://www.markdownguide.org/cheat-sheet/ is what I want to do, btw. Notice how the page nav is positioned after the header, and only fixes itself to the viewport when the page scrolls down.

Also, try increasing the size of the header using the web inspector. You'll notice that the page navigation will also move downwards.

Their document structure and CSS is pretty well thought out :-)

@pyokagan
Copy link

pyokagan commented Feb 8, 2019

One of the more straightforward first steps might be to add header support to page layouts. The header will be inserted before the page nav/site nav/page content. This ensures we can use normal flow to position the page nav / site nav / page content below the header, without needing to resort to magic numbers.

diff --git a/src/Page.js b/src/Page.js
index aa24ba6..345bdc9 100644
--- a/src/Page.js
+++ b/src/Page.js
@@ -23,6 +23,7 @@ const LAYOUT_DEFAULT_NAME = 'default';
 const LAYOUT_FOLDER_PATH = '_markbind/layouts';
 const LAYOUT_FOOTER = 'footer.md';
 const LAYOUT_HEAD = 'head.md';
+const LAYOUT_HEADER = 'header.md';
 const LAYOUT_NAVIGATION = 'navigation.md';
 const NAVIGATION_FOLDER_PATH = '_markbind/navigation';
 
@@ -550,6 +551,26 @@ Page.prototype.removeFrontMatter = function (includedPage) {
   return $.html();
 };
 
+/**
+ * Inserts the page layout's header to the start of the page
+ * @param pageData a page with its front matter collected
+ */
+Page.prototype.insertHeader = function (pageData) {
+  const headerFile = path.join(LAYOUT_FOLDER_PATH, this.frontMatter.layout, LAYOUT_HEADER);
+  const headerPath = path.join(this.rootPath, headerFile);
+  if (!fs.existsSync(headerPath)) {
+    return pageData;
+  }
+  // Retrieve Markdown file contents
+  const headerContent = fs.readFileSync(headerPath, 'utf8');
+  // Set header file as an includedFile
+  this.includedFiles[headerPath] = true;
+  // Map variables
+  const newBaseUrl = calculateNewBaseUrl(this.sourcePath, this.rootPath, this.baseUrlMap) || '';
+  const userDefinedVariables = this.userDefinedVariablesMap[path.join(this.rootPath, newBaseUrl)];
+  return `${nunjucks.renderString(headerContent, userDefinedVariables)}\n${pageData}`;
+};
+
 /**
  * Inserts the footer specified in front matter to the end of the page
  * @param pageData a page with its front matter collected
@@ -784,6 +805,7 @@ Page.prototype.generate = function (builtFiles) {
       .then(result => addContentWrapper(result))
       .then(result => this.insertPageNavWrapper(result))
       .then(result => this.insertSiteNav((result)))
+      .then(result => this.insertHeader(result))
       .then(result => this.insertFooter(result)) // Footer has to be inserted last to ensure proper formatting
       .then(result => formatFooter(result))
       .then(result => markbinder.resolveBaseUrl(result, fileConfig))
diff --git a/src/Site.js b/src/Site.js
index 4c884ac..d90ca51 100644
--- a/src/Site.js
+++ b/src/Site.js
@@ -44,7 +44,7 @@ const SITE_CONFIG_NAME = 'site.json';
 const SITE_DATA_NAME = 'siteData.json';
 const SITE_NAV_PATH = '_markbind/navigation/site-nav.md';
 const LAYOUT_DEFAULT_NAME = 'default';
-const LAYOUT_FILES = ['navigation.md', 'head.md', 'footer.md', 'styles.css'];
+const LAYOUT_FILES = ['navigation.md', 'head.md', 'header.md', 'footer.md', 'styles.css'];
 const LAYOUT_FOLDER_PATH = '_markbind/layouts';
 const LAYOUT_SCRIPTS_PATH = 'scripts.js';
 const LAYOUT_SITE_FOLDER_NAME = 'layouts';

@pyokagan
Copy link

pyokagan commented Feb 8, 2019

The header will be inserted before the page nav/site nav/page content.

... Or, that's the plan, but that's currently not possible since the current code makes it such that the page nav is always first.

So, the next step might be to decouple the position of the page nav in the generated HTML from when it is generated in the code. The page nav needs to be rendered last because it can only examine the headers in the page right at the end (to take into account e.g. headers hidden by tags).

One way around it might be to add a placeholder page nav at the correct position in the document, and then replace it with the final rendered page nav at the end using nunjucks:

diff --git a/src/Page.js b/src/Page.js
index 345bdc9..55c32a5 100644
--- a/src/Page.js
+++ b/src/Page.js
@@ -34,6 +34,7 @@ const FLEX_DIV_ID = 'flex-div';
 const FRONT_MATTER_FENCE = '---';
 const PAGE_CONTENT_ID = 'page-content';
 const PAGE_NAV_CONENT_WRAPPER_ID = 'page-nav-content-wrapper';
+const PAGE_NAV_PLACEHOLDER_NAME = 'markbindPageNavPlaceholder';
 const SITE_NAV_ID = 'site-nav';
 const TITLE_PREFIX_SEPARATOR = ' - ';
 
@@ -645,6 +646,14 @@ Page.prototype.insertSiteNav = function (pageData) {
     + '</div>';
 };
 
+/**
+ * Inserts placeholder where the rendered page nav would be placed.
+ */
+Page.prototype.insertPageNavPlaceholder = function (pageData) {
+  // <!-- --> needed to force a HTML block.
+  return `<!-- -->{{ ${PAGE_NAV_PLACEHOLDER_NAME} | safe }}\n${pageData}`;
+};
+
 /**
  *  Inserts wrapper for page nav contents CSS manipulation
  */
@@ -725,9 +734,9 @@ Page.prototype.generatePageNavTitleHtml = function () {
 };
 
 /**
- *  Insert page navigation bar with headings up to headingIndexingLevel
+ * Renders page navigation bar with headings up to headingIndexingLevel
  */
-Page.prototype.insertPageNav = function () {
+Page.prototype.renderPageNav = function () {
   if (this.isPageNavigationSpecifierValid()) {
     const $ = cheerio.load(this.content);
     this.navigableHeadings = {};
@@ -740,8 +749,9 @@ Page.prototype.insertPageNav = function () {
       + `    ${pageNavHeadingHTML}\n`
       + '  </nav>\n'
       + '</nav>\n';
-    this.content = htmlBeautify(`${pageNavHtml}\n${this.content}`, { indent_size: 2 });
+    return pageNavHtml;
   }
+  return '';
 };
 
 Page.prototype.collectHeadFiles = function (baseUrl, hostBaseUrl) {
@@ -808,6 +818,7 @@ Page.prototype.generate = function (builtFiles) {
       .then(result => this.insertHeader(result))
       .then(result => this.insertFooter(result)) // Footer has to be inserted last to ensure proper formatting
       .then(result => formatFooter(result))
+      .then(result => this.insertPageNavPlaceholder(result))
       .then(result => markbinder.resolveBaseUrl(result, fileConfig))
       .then(result => fs.outputFileAsync(this.tempPath, result))
       .then(() => markbinder.renderFile(this.tempPath, fileConfig))
@@ -822,8 +833,11 @@ Page.prototype.generate = function (builtFiles) {
 
         this.addLayoutFiles();
         this.collectHeadFiles(baseUrl, hostBaseUrl);
-        this.content = nunjucks.renderString(this.content, { baseUrl, hostBaseUrl });
-        this.insertPageNav();
+        this.content = nunjucks.renderString(this.content, {
+          baseUrl,
+          hostBaseUrl,
+          [PAGE_NAV_PLACEHOLDER_NAME]: this.renderPageNav(),
+        });
         return fs.outputFileAsync(this.resultPath, this.template(this.prepareTemplateData()));
       })
       .then(() => {

This would then allow the page nav to be placed anywhere in the document.

@Chng-Zhi-Xuan
Copy link
Contributor

Chng-Zhi-Xuan commented Feb 11, 2019

Thanks @pyokagan for taking your time to craft a solution for this issue. I also agree that my initial implementations didn't have a cohesive design with each other (page-nav / site-nav).

There was a discussion to implement display: flex containers, one for each section of the Holy grail layout. The position: sticky was not considered initially but you bring up valuable design points, maybe we can try to implement optional sticky behaviour individually for each of the Holy grail sections.

Stating all of the above, it will not be a trivial implementation. So bear with us until we can push the PR out 😅.

@pyokagan
Copy link

There was a discussion to implement display: flex containers, one for each section of the Holy grail layout. The position: sticky was not considered initially but you bring up valuable design points, maybe we can try to implement optional sticky behaviour individually for each of the Holy grail sections.

Yes, I think implementing display: flex containers would be a good idea. The page structure should work well without requiring position: fixed or position: sticky.

But maybe the underlying problem is that there is no templating system (despite there being a page.ejs which is not exposed), and that a quite a bit of the output HTML is generated manually inside Page.js...

@Chng-Zhi-Xuan Chng-Zhi-Xuan self-assigned this Feb 22, 2019
@Chng-Zhi-Xuan Chng-Zhi-Xuan added f-SiteNav f-PageNav and removed s.UnderDiscussion The team will evaluate this issue to decide whether it is worth adding labels Feb 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants