Support page top header #733
Conversation
|
CS3282 Team members are welcome to provide reviews 😄 Since the |
|
Headings are only collected within the content wrapper after the page is finished generating. It will exclude page-nav, site-nav, header and footer sections as they will have been separated from the content before the file write. (From collection methods)
I did not change anything other than removing inline footers, so existing behaviour (not affecting) should be preserved for file footers.
This is a great idea, but would require us to pre-parse all As the changes required to |
| /** | ||
| * Inserts the footer specified in front matter to the end of the page | ||
| * @param pageData a page with its front matter collected | ||
| */ | ||
| Page.prototype.insertFooter = function (pageData) { | ||
| Page.prototype.insertFooterFile = function (pageData) { |
There was a problem hiding this comment.
Was thinking if this can be refactored into a plugin? Similar to #714 ? Just a side thought though.
There was a problem hiding this comment.
Will take a look if it is feasible.
Although I am thinking that specifying plugins is less user intuitive compared to the current implementation of specifying via <frontmatter> or using layouts.
There was a problem hiding this comment.
We already pass frontMatter to plugins right?
|
@Chng-Zhi-Xuan Thanks so much for taking a look at this 👍 As an aside, seems like the site nav links in https://deploy-preview-733--markbind-master.netlify.com/userguide/ are broken. |
yup, thanks for tackling this major feature @Chng-Zhi-Xuan |
| Page.prototype.insertHeaderFile = function (pageData) { | ||
| const { header } = this.frontMatter; | ||
| let headerFile; | ||
| if (header) { |
There was a problem hiding this comment.
Just wondering - could header be an empty string'' (this would resolve to false)? If so, should do a check for header === undefined here instead to be more explicit?
There was a problem hiding this comment.
Yes, it could be an empty string but the user would have to explicitly specify a header field in <frontmatter> and leave it empty.
Also, empty string is a falsy value, so it will still go to the else statement.
Note that the first statement is to check for any value specified within <frontmatter>, the logic of header === undefined is not correct (since it will be false when a valid string is given).
There was a problem hiding this comment.
Sorry, I think it should have been header !== undefined instead, but if the way we handle an empty header is the same as the way we handle the case where no header is defined, it might not be necessary.
Thanks for the clarification! 🙂
src/Page.js
Outdated
| this.content = htmlBeautify($.html(), { indent_size: 2 }); | ||
| }; | ||
|
|
||
| Page.prototype.collectSiteNavigation = function () { |
There was a problem hiding this comment.
This method seems rather similar to collectHeader. Would it make sense to abstract this behaviour into another method?
There was a problem hiding this comment.
Same applies for the collectFooter method.
There was a problem hiding this comment.
Now that you mentioned it, we could abstract it to
Page.prototype.collectPageSection = function (section) {
const $ = cheerio.load(this.content, { xmlMode: false });
const selectedSection = $(section)
....
this.pageSections[section] = $.html(section);
}
Page.collectAllPageSection = function () {
this.collectPageSection('header');
this.collectPageSection(`#${SITE_NAV_ID}`);
this.collectPageSection('footer');
}| <div id="app"> | ||
| <div id="content-wrapper"> | ||
| <div id="app"> | ||
|
|
There was a problem hiding this comment.
Just wondering if the empty lines here are intended? 😅
There was a problem hiding this comment.
Most likely due to the line break between template variables within Page.ejs, might look into it (since it is kind of ugly :P)
@pyokagan , thanks for helping me find a bug! Due to the positioning of this.content = nunjucks.renderString(this.content, { baseUrl, hostBaseUrl });Likewise for the other collected sections. |
| jQuery(window).on('activate.bs.scrollspy', (event, obj) => { | ||
| document.querySelectorAll(`a[href='${obj.relatedTarget}']`).item(0).scrollIntoView(false); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Are you planning any workarounds to restore this feature?
There was a problem hiding this comment.
With the new layout, the sticky page-nav now has the height of the <content> instead of the viewport. So scrollIntoView would not work.
A different work around would need to be implemented, which is an independently scrollable sticky element.
References
Scrollable Sticky Element
Sticky Sidebar
a5d1882 to
99dece4
Compare
|
Update
Ready for review
Would like reviewers to focus on the following:
To Do (Separate PR?)
|
|
Requesting for CS3282 Team Members Review @Xenonym @marvinchin @amad-person @sijie123 @luyangkenneth This is a large feature PR, so it is good exposure to look through and comment/review it :) |
asset/css/page-nav.css
Outdated
| } | ||
|
|
||
| /* Responsive site navigation */ | ||
|
|
There was a problem hiding this comment.
Is the number of blank lines here intended?
There was a problem hiding this comment.
Not intended, looks like eslint did not complain about this :P
There was a problem hiding this comment.
ESLint doesn't check CSS files. 🙈
There was a problem hiding this comment.
Is there any interest in adding a linter for css? (e.g. https://stylelint.io) 🙂
There was a problem hiding this comment.
Is there any interest in adding a linter for css? (e.g. https://stylelint.io) 🙂
@marvinchin PR is welcome for a css linter.
asset/css/markbind.css
Outdated
| @@ -78,12 +78,53 @@ code.hljs.inline { | |||
| background-color: #fff; | |||
| } | |||
|
|
|||
| /** Holy Grail Layout **/ | |||
There was a problem hiding this comment.
Just wondering if the/** is intended instead of /*? I can't seem to find any other comments that use this convention in the codebase 😅
There was a problem hiding this comment.
Because it was intended for /** Holy Grail Layout **/ to be the main section and for /* Header */, /* Footer */ etc to be sub-sections under the main section.
I agree that it is subtle and not intuitive. Might change the commenting style to something more widely recognised.
There was a problem hiding this comment.
After digging around, this seems much better :)
/**
* Holy Grail Layout
*
* This section covers the common styles used within Header, Footer and Side Navigation bars
*/| <div id="app"> | ||
| <div id="content-wrapper"> | ||
| <div id="app"> | ||
|
|
There was a problem hiding this comment.
There seems to be a number of extraneous blank lines in the output html. Is this intended?
There was a problem hiding this comment.
I have not looked into this in depth but I might suspect it is due to prepareTemplateData defaulting to empty string values.
I should test using the <%_ if %> syntax to avoid inserting empty strings and see if there are improvements.
There was a problem hiding this comment.
Update
Tested with <% if (...) %> syntax but does not get rid of the blank lines.
However, adding either -%> or _%> to the end of the template variable insertion works.
Interesting finding:
The indentation before the template variables is only applied to the first line of the inserted html instead of all the lines within the inserted html. I would assume page.ejs is not smart enough to tell apart simple strings vs proper html :P
| <div class="text-center"> | ||
| Default footer | ||
| </div> | ||
| </footer> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
The indentation here seems off too
There was a problem hiding this comment.
Might be due to the indentation within page.ejs. Seems like a legacy issue since we have been using 4 space indentation within page.ejs but 2 space indentation for htmlBeautify.
src/Page.js
Outdated
| @@ -150,7 +131,7 @@ function formatSiteNav(renderedSiteNav, src) { | |||
| return renderedSiteNav; | |||
| } | |||
| // Tidy up the style of the unordered list <ul> | |||
| listItems.parent().attr('style', 'list-style-type: none; margin-left:-1em'); | |||
| listItems.parent().addClass('site-nav-list'); | |||
There was a problem hiding this comment.
Should we move site-nav-list to a constant? (similar to page-nav)
There was a problem hiding this comment.
Actually this brings up an interesting point.
Relating to Webstorm IDE (maybe other IDEs as well), where if you refactor CSS names, it will automatically change the name of that class within html strings or function parameters that take in CSS names. However, constant strings with the same name will not be refactored.
Also, we shouldn't use a constant for a string if it is only used once throughout the whole file.
So we should be finding single use constants and changing them back to be used inline? (site-nav-list only appears once)
There was a problem hiding this comment.
Hmm, I would say that there's still value in refactoring it even if it is a single use now, because in case in the future we need to use this somewhere else in the code, we need not do the refactoring again (which is also provided that we even realise that the same class is used here. If we forgot about it EDIT: because not everyone uses IDE, then we are using "magic values" everywhere).
| + '</nav>\n'; | ||
|
|
||
| return `${wrappedSiteNav}` | ||
| + `${pageData}`; |
There was a problem hiding this comment.
Would return `${wrappedSiteNav}${pageData}`; work instead? 🙂
There was a problem hiding this comment.
Yes, it would work. However, within wrappedSiteNav there is a line break at the end of the string. So I wrote it in this manner to visually represent that wrappedSiteNav and pageData is separated by one line break.
There was a problem hiding this comment.
We can move the \n out of wrappedSiteNav to here, so that the visual representation is consistent.
There was a problem hiding this comment.
Thanks for the PR @Chng-Zhi-Xuan!
I'm not very well versed with CSS, so I can't accurately tell whether the added CSS formatting work correctly in achieving the intended outcome. However, I previewed the netlify site and it looks pretty okay to me.
Edit: Hmm there seems to be a big gap here :O
The gap goes away when I start scrolling. I presume it's something to do with the sticky display?
For my review, I will focus more on the back-end side of parsing things. Other than the extra indentations probably caused by the beautifier (thanks @marvinchin!), I just have one small comment w.r.t Page.js
There's also something that I noticed, but it's not relevant to this PR. I'll open up a new issue in a bit.
| } | ||
| const headerPath = path.join(this.rootPath, headerFile); | ||
| if (!fs.existsSync(headerPath)) { | ||
| return pageData; |
There was a problem hiding this comment.
Should we log an error here?
Otherwise, there would be no header content but the user won't know why.
There was a problem hiding this comment.
Since headers and footers are optional, an explicit error should not be logged.
To add on, other optional items such as site-nav and page-nav are also not logged if they are not added.
Furthermore, if we do log it then the console will be filled with messages if there are a significant amount of pages with missing header files.
Although I can see explicit logging useful for debugging purposes. E.G.
header file skipped: header file not found
page navigation added: {Title: Test Title} {headingIndexingLevel: 2}
footer file skipped: footer not specified, layout not specifiedBut alas, not in the scope of this PR :)
|
@sijie123 , Thanks for looking through. To reply the CSS part: .spacer-top {
/*
spacer-top class is a work around for MarkBind <navbar> component using "position: fixed".
This class can be removed once the component's position type is changed.
*/
padding-top: 70px;
}Both |
1e7b75f to
2072f33
Compare
|
Update Review Amendments:
Modifications:
Documentation links Follow up:
|
2072f33 to
10c2db5
Compare
|
Update
|
10c2db5 to
4afdbda
Compare
|
Update
|
docs/userGuide/syntax/headers.mbdf
Outdated
|
|
||
| **You can specify a <tooltip content="Content to be placed at the top of a page.">page header</tooltip>** above your page contents and <tooltip content="Site Navigation and <br>Page Navigation menus">navigation menus</tooltip> using a `header file`. | ||
|
|
||
| You can save multiple `header files` in the `_markbind/headers` folder and specify it in the `<frontmatter>` of the pages in which it should appear. |
There was a problem hiding this comment.
The use of code rendering for the words header files and footer files looks out of place.
Change header files -> header files? And do the same for footer files.
| @@ -94,9 +95,19 @@ const FOOTER_DEFAULT = '<footer>\n' | |||
| + ' </div>\n' | |||
| + '</footer>\n'; | |||
|
|
|||
| const HEADER_DEFAULT = '<header>\n' | |||
| + ' <div class="bg-primary display-4 text-center text-white">\n' | |||
There was a problem hiding this comment.
This is subjective, but the default header generated seems to be too dominating compared to the rest of the parts of the webpage. :P
This is how it looks like now with a markbind init website:
There was a problem hiding this comment.
Intended to be a large banner / Jumbotron. Similar to the one in Markdown Guide.
Also, display-4 is the smallest banner text on Bootstrap without adding additional styles into .scss ( >_<)
| @@ -1,58 +1,64 @@ | |||
| <!DOCTYPE html> | |||
| <html> | |||
|
|
|||
There was a problem hiding this comment.
The test site doesn't seem to have a page that uses our new headers.
There was a problem hiding this comment.
I avoided placing a Jumbotron on the test site as the current top nav bar will block it.
After a PR is done to modify <navbar> in the vue-strap repo, we require another PR here to update our usage of <navbar>. Within that PR, we can add testing header elements.
The current functionality of the site nav expand button is to change the width of site nav to either hide or show it on smaller screen widths. Under the new layout, changing the width of any side navigation menus will result in content shifting within the main content page. This is not reader friendly so let's remove the functionality.
Inline styles cannot be overwritten by authors, so let's refactor them into classes for easy overwritting.
Modifications to the previous implementation of site nav is necessary for compatibility with the new holy grail layout.
Using position sticky breaks the functionality of scrollIntoView() so let's remove it.
Modifications to the previous implementation of page nav is necessary for compatibility with the new holy grail layout. page nav wrapper was removed as altering the margin of the page content is no longer necessary.
With the new template requiring the page sections to be individually accessable via a variable, a collection method is necessary. Another benefit is that it decouples the page content from any other page section elements.
Most of the layout styles that were related to flexbox has been ported to markbind.css as site-nav and page-nav both require it.
The behaviour of inline footers has changed in this pull request. Let's update the documentation to reflect that.
As Headers is a new feature supported in this pull request, we should have documentation to describe how it works.
Within Page.ejs, it has indentations before insertion of template variables to represent the holy grail layout. However, due to limitations of the .ejs API, the additional white spaces can't be trimmed if a certain template variable was not inserted. Let's call htmlBeautify after the output html is fully generated before doing a file write to ensure consistent formatting.
3e3b581 to
eb21d5f
Compare
|
@Chng-Zhi-Xuan What if I want to have the same navbar in multiple pages but I also want a different jumbotron in each page e.g., https://www.markdownguide.org? Do I need to create many different header files and duplicate the navbar code in each (as include is not allowed in header files)? |
|
With respect to the above, why not allow the header to be cumulative, in the order they are specified? |
Unfortunately yes, you have to have multiple header files if the content is different as there is no way to add content to the header file outside of authoring it. Although supporting includes within This was mentioned by Aaron above,
I found a relevant discussion, "Should header and footer tags be inside main tag?". Unless the inline Consecutive header tags are supported in the current implementation, as long as it is within the same header file. <!-- within my-header.md -->
<header class"top-nav-stuff">
...
</header>
<header class="jumboBlue">
...
</header> |
|
What I'm trying to do is something like this: <frontmatter>
header: top-nav.md
</frontmatter>
<header class="jumboBlue">
...
</header>That way, I can put the topnav in the header file (as it is common to all pages) and then add on a page specific-jumbotron below it in each page. The documentation says inline headers will be removed. Do we even support inline headers? The docs doesn't say how to use an inline header. |
|
@damithc Sorry, missed this comment
This is definitely possible. but can be confusing for the user. E.G <!-- Input by author -->
<frontmatter>
header: shared-header.md
<frontmatter>
<header class=jumboBlue>
Text 1
</header>
Some content 1
<header class="detailed-desc">
Description text
</header>
Some content 2 <!-- Output by MarkBind -->
<header>
sharedHeader
</header>
<header class="JumboBlue">
Text 1
</header>
<header class="detailed-desc">
Description text
</header>
<div id="content-wrapper">
Some Content 1
Some Content 2
</div>As we have to shift every header outside the Alternatives to this are less elegant than enforcing strict use of footer/header files:
Existing documentation specified use of Inline footers but was removed. It was supported prior to this PR. Specifically it was for inline footers. Where inline footers will override specified footers. This feature has been removed, and the new behaviour is ported over to headers as well, thus the notes being almost identical. I would agree that adding some context of what are inline headers and footers to the user guide would be better. |
It is strange to explain a feature that no longer works though. |
It is not really a feature, but something that can be created by the author. As inline headers and footers (within the |
Oh? Didn't realize they are normal HTML tags. In that case shouldn't we have used some other tags e.g., |
|
I would like to clarify what would be the purpose of a custom tag for headers? (For inline headers or for the tag used within Since functionally there is no special operations being performed on |
If there is no difference, then it makes sense to use the same tag. 👍
I can live with that, as it would solve my immediate problem. :-) Alternatively, we can provide some attribute (or a different tag) to differentiate between the two types of headers. What do you think? |
|
A different tag would be appropriate (since we will be shifting the position of that tag) Implementation should be simple, by using Cheerio to select and shift the headers with that specific tag to the top. It will be appended sequentially and keeping any classes authored on it. |
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [x] New feature
• [x] Enhancement to an existing feature
Resolves #636
Resolves #549
Fixes #755
What changes did you make? (Give an overview)
site-navandpage-navare now in the sameflex-body-divas the content, and are usingposition: stickyto have it follow the user as they are scrolling through content.site-nav.cssandpage-nav.cssto the#content-wrapperID withinmarkbind.css.site-nav,header,footer) is appended to the page content so that we only call MarkBind parser once.Removals (Justification in Discussion)
site-navbuttonscrollIntoViewImplementation Discussion
In the new layout, expanding
site-navusing the button (on smaller screens) will cause the content to shift, causing stuttering of the page.position: fixedinstead of flex box when screen is small.Maintain existing behaviour. However, when transitioning from
position: stickytoposition: fixedhas stuttering and animation issues.This solution becomes complex if we want to automate this, by inserting into an existing top nav or to render a top nav responsively if there isn't any.
Removed
scrollIntoViewwithinsetup.js#pageNavSetup()position: stickyelementsRemove any inline
<header>or<footer>elementsindex.mdfile should contain content for the<main>part of the page, and should not overlap with<header>or<footer>content-wrapperfor searching and indexing headingsIs there anything you'd like reviewers to focus on?
Site.test.jsFootersandHeadersoverflow: hiddenbreaksposition: stickyTesting instructions:
markbind servein the directory