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 content insertion into <head> of a Page #361

Merged
merged 6 commits into from
Jul 25, 2018

Conversation

Chng-Zhi-Xuan
Copy link
Contributor

@Chng-Zhi-Xuan Chng-Zhi-Xuan commented Jul 23, 2018

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

• [x] New feature

Resolves #355

What is the rationale for this request?
User would like to easily insert their own script or css files into the <head> of a page.

What changes did you make? (Give an overview)

  • Have Site initialise a new head folder within the _markbind folder.
  • Insert logic for Page to process the specified head file in front-matter
  • Update page.ejs template to insert content from head file
  • Update Site unit tests
  • Add code and files to test content insertion within test_site
  • Add description to user guide
// Within customScript.js
console.log('custom script inserted into the head successfully!');
In console In Page HTML
355-demo-1 355-demo-2

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

  • Method / Variable naming
  • Whether is there a need to do nunjucks.renderString within collectHeadFiles
  • User Guide wording

Testing instructions:

  1. Checkout this PR's branch and navigate to test_site folder.
  2. Run markbind serve and check console for the success message from inserted script
  3. Add new folders with CSS or JS files in the root
  4. Use the appropriate syntax and add them inside myCustomHead.md
  5. Run markbind serve and you should see the <head> having your newly authored content from myCustomHead.md along with the files in Sources tab (in Chrome).

@Chng-Zhi-Xuan Chng-Zhi-Xuan changed the title [WIP] Support content insertion into <head> of a Page [WIP] Support content insertion into <head> of a Page Jul 23, 2018
@Chng-Zhi-Xuan Chng-Zhi-Xuan changed the title [WIP] Support content insertion into <head> of a Page Support content insertion into <head> of a Page Jul 24, 2018
@Chng-Zhi-Xuan Chng-Zhi-Xuan requested a review from yamgent July 24, 2018 02:04
@Chng-Zhi-Xuan
Copy link
Contributor Author

Chng-Zhi-Xuan commented Jul 24, 2018

Update

  • Add user guide section
  • Update PR title and description
  • Ready for review

lib/Page.js Outdated
const userDefinedVariables = this.userDefinedVariablesMap[path.join(this.rootPath, newBaseUrl)];
const headFileMappedData = nunjucks.renderString(headFileContent, userDefinedVariables);
this.headFileReferences = headFileMappedData.trim();
return pageData;
Copy link
Member

Choose a reason for hiding this comment

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

Seems like the pageData is not used at all in this method, so it is not necessary.

Therefore, we can call this method before the line this.headFileReferences = nunjucks.renderString(this.headFileReferences, { baseUrl, hostBaseUrl }); instead of doing .then(result => this.collectHeadFiles(result)).

lib/Page.js Outdated
// Map variables
const newBaseUrl = calculateNewBaseUrl(this.sourcePath, this.rootPath, this.baseUrlMap) || '';
const userDefinedVariables = this.userDefinedVariablesMap[path.join(this.rootPath, newBaseUrl)];
const headFileMappedData = nunjucks.renderString(headFileContent, userDefinedVariables);
Copy link
Member

Choose a reason for hiding this comment

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

Is this rendering necessary? Seems to still work for me when I commented it out. Also the render method seems to be called again below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is useful if the user predefined a <style> or <link> variable and used it in headFile.md.

The render method below is specifically for resolving {{baseUrl}}.

@@ -375,6 +375,50 @@ Front matter can also be included from a separate file, just like how content ca

This will result in `index.md` having the title `Binary Search Tree` and the specified keywords in order for it to be looked up through the search bar.

### Inserting content into a page's head element

While authoring your website, you may want to have your own CSS or Javascript file to be included in a page.
Copy link
Member

Choose a reason for hiding this comment

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

file -> files

- More than one head file can be created for different pages.

- Author your `<style>` elements for CSS files and `<link>` elements for Javascript files using HTML as shown below.
- Ensure that any url starts from the root directory <code>{<span></span>{baseUrl}}/</code> when you are referencing your files.
Copy link
Member

Choose a reason for hiding this comment

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

Ensure that any url starts... -> Ensure that your URLs start...
... from the root directory {{baseUrl}} ... -> ... from the root directory, by using {{baseUrl}} ...

<script src="{{baseUrl}}/yourScriptFolder/myCustomScript.js"></script>
```

- Specify the head file in pages that you want it, within the [front matter](#front-matter) `head` attribute.
Copy link
Member

Choose a reason for hiding this comment

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

...that you want it, within the... -> ...that uses it, by specifying the...

```

The head file contents will be placed near the end of the page's head tag.
It will override existing Bootstrap and MarkBind CSS styles if there is an overlap.
Copy link
Member

Choose a reason for hiding this comment

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

It will... -> Your head file will...
there is an overlap -> there is an overlap of classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... overlap of classes -> ... overlap of selectors

Doc here

@yamgent
Copy link
Member

yamgent commented Jul 24, 2018

Also by any chance, is it possible to support live preview for changes to the head files?

@Chng-Zhi-Xuan
Copy link
Contributor Author

Update

  • Apply changes requested
  • Add support for page regeneration on head file change


```html
<!-- In _markbind/head/compiledRef.md -->
<link rel="stylesheet" href="{{baseUrl}}/yourCSSFolder/subfolder/myCustomStyle.css">
Copy link
Member

Choose a reason for hiding this comment

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

{{baseUrl}} is not getting rendered (see Netlify preview).

@yamgent
Copy link
Member

yamgent commented Jul 25, 2018

@Chng-Zhi-Xuan the commits need to be squashed.

As discussed with Prof, we can live with the {{baseUrl}} documentation problem for now, and fix it in a separate PR (in fact, another section also have the {{baseUrl}} documentation problem, so might as well solve everything later).

Will merge it as soon as it is squashed.

@Chng-Zhi-Xuan Chng-Zhi-Xuan force-pushed the 355-insert-head-files branch from d8830c8 to 7c7600f Compare July 25, 2018 02:40
@Chng-Zhi-Xuan
Copy link
Contributor Author

Update

  • Squashed commits

@yamgent yamgent added this to the v1.9.2 milestone Jul 25, 2018
@yamgent yamgent merged commit 97711ef into MarkBind:master Jul 25, 2018
@damithc
Copy link
Contributor

damithc commented Jul 25, 2018

@Chng-Zhi-Xuan I'm having trouble getting this to work in website-base. Can you try it out?
Attached files:

  • _/markbind/head/scheduleHead.md
  • schedule/index.md

head files.zip

I got the feature to work in other places. In fact I can include scheduleHead.md as the head in other files. But when I include it in schedule/index.md, it doesn't get inserted into the head.

@damithc
Copy link
Contributor

damithc commented Jul 25, 2018

To add to the above, full code available in the scheduleHead branch https://github.com/nus-cs2103/website-base/tree/scheduleHead

@Chng-Zhi-Xuan
Copy link
Contributor Author

@damithc

Findings:

  • Clicking on "Schedule" in the main page's navbar directs to /website-base/index.html, which does not specify any head file in the front-matter.

  • Navigating to /website-base/schedule/index.html and it has the specified head file as shown below.

361-head-content

This is intended behaviour as <include> ignores the file's front-matter.

A quick fix is to specify scheduleHead.md in the main page's index.md 😄

@damithc
Copy link
Contributor

damithc commented Jul 25, 2018

Indeed that should be the problem. My bad. Sorry. Thanks for the help.

@Chng-Zhi-Xuan Chng-Zhi-Xuan deleted the 355-insert-head-files branch May 21, 2019 06:40
Sign up for free to 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.

Support google analytics
3 participants