Uh oh!
There was an error while loading. Please reload this page.
Fix site-nav.css not being added when layout contains site nav - #841
Conversation
I briefly looked through, here are some comments
In my opinion, if the user has a We might also want to insert notes / tips into the Implementation comments
To streamline this, would it be better if you change a global boolean variable after This will only require you to call |
marvinchin
commented
Apr 15, 2019
Thanks for pointing that out! I was trying to avoid the use of a global flag because it introduces some coupling between places that modify the flag, and those that read it (in this case, between As |
eb3d908 to
f16e9b5Comparemarvinchin
commented
Apr 15, 2019
After updating the logic to use a flag instead, we are able to avoid this issue as we only update the flag after checking that the file is not empty. |
Chng-Zhi-Xuan
left a comment
There was a problem hiding this comment.
After updating the logic to use a flag instead, we are able to avoid this issue as we only update the flag after checking that the file is not empty.
👍 , just have a few comments below.
| if (siteNavContent === '') { | ||
| return pageData; | ||
| } | ||
| this.hasSiteNav = true; |
There was a problem hiding this comment.
Since you only modify the flag to be true at this point. If you delete the navigation.md (Or blank the contents) from the layouts folder, the site-nav will still appear in the markbind serve preview. Even though MarkBind correctly detected navigation.md has changed.
The only correct behaviour in this code is changing navigation.md slightly and the changes will reflect in the markbind serve preview.
Suggestion
Explicitly set the flag to false in the if statements above for it to be properly removed when navigation.md is deleted / blanked in preview mode.
| this.navigableHeadings = {}; | ||
| this.pageSectionsHtml = {}; | ||
| // flag to indicate whether this page has a site nav |
There was a problem hiding this comment.
A quick search shows majority of inline comments have capitalised starting character (although not all).
When passing data to the page template, the siteNav variable only checks for the existence of the site nav specified in the frontmatter. The site-nav.css file is thus only added when the site nav is specified in the frontmatter. When a page has a layout with a site nav, but no site nav is specified in the frontmatter, the site-nav.css file is not added, which causes the styling of the site nav to break. Let's update the logic when forming template data to check for the existence of a site nav in the template when determining the value of the siteNav variable.
f16e9b5 to
2f3a48cComparemarvinchin
commented
Apr 16, 2019
@Chng-Zhi-Xuan I've made the requested changes! |
damithc
commented
Apr 23, 2019
Seems to be working. Good work! |
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [x] Bug fix
Fixes#528.
What is the rationale for this request?
When a page has a site nav specified in the layout, but not in the frontmatter, the site-nav.css file is not added to the site. This causes the styling of the side nav to break.
What changes did you make? (Give an overview)
insertSiteNavgetSiteNavPathto check if a site nav is specified in either layout or frontmatter when preparing template dataIs there anything you'd like reviewers to focus on?
By default, the default layout contains a blanknavigation.mdfile. The site-nav.css file is thus added even though thenavigation.mdfile is empty - is this behaviour okay?After updating the logic to use a flag instead, we are able to avoid this issue as we only update the flag after checking that the file is not empty.
Testing instructions:
See instructions in #739. The site nav should be styled correctly.
Proposed commit message: (wrap lines at 72 characters)
When passing data to the page template, the siteNav variable only checks
for the existence of the site nav specified in the frontmatter. The
site-nav.css file is thus only added when the site nav is specified in
the frontmatter.
When a page has a layout with a site nav, but no site nav is specified
in the frontmatter, the site-nav.css file is not added, which causes the
styling of the site nav to break.
Let's update the logic when forming template data to check for the
existence of a site nav in the template when determining the value of
the siteNav variable.