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

Fix frontmatter not live-reloading on header/footer update #1035

Closed
wants to merge 1 commit into from
Closed

Fix frontmatter not live-reloading on header/footer update #1035

wants to merge 1 commit into from

Conversation

Parcly-Taxel
Copy link
Contributor

@Parcly-Taxel Parcly-Taxel commented Feb 9, 2020

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

• [X] Bug fix

This one-line update fixes #1001 using the suggested fix provided here.

What is the rationale for this request?
This is also my first pull request to this project.

Testing instructions:
See the issue opener on #1001.

Proposed commit message: (wrap lines at 72 characters)
Fix reload inconsistency when updating frontmatter

@Parcly-Taxel Parcly-Taxel requested a review from yamgent February 10, 2020 04:10
@yash-chowdhary
Copy link
Contributor

Thanks for your PR @Parcly-Taxel!
As mentioned in #1001's comments - 1 and 2, we can also go ahead with the other suggested fix of filling the header.md and footer.md files to make sure they're not blank.

@Parcly-Taxel
Copy link
Contributor Author

Parcly-Taxel commented Feb 11, 2020

As mentioned in #1001's comments, we can also go ahead with the other suggested fix of filling the header.md and footer.md files to make sure they're not blank.

@yamgent I've made the suggested fix. Please review.

With the current diff utilities it is impossible to ignore changing content like the one in MarkBind's default footer. I had to change diffHtml.js: see here.

@@ -9,7 +9,9 @@ const DiffPrinter = require('./diffPrinter');
* @returns {bool} if diff was found
*/
const diffHtml = (expected, actual, filePathName) => {
const diffParts = jsdiff.diffWords(expected, actual);
// ignore default template's footer
const timeRegex = /\[Generated by .*? on .*?\]/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use an empty footer instead?
This change may cause text explicitly typed by the user that matches the regex to not be detected.

Looks good otherwise 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that line is not present, five of the tests fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, because {{ timestamp }} is present in the footer, those tests would fail.
You could make the footer empty ( but keep the <footer> tag ) to fix it

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed src/template/minimal has the same footer/header/navgation.md files too, do update those as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ang-zeyu I only needed to take out one line to make all the tests pass (i.e. the footer remains non-empty). Please re-review.

And by the way, what files am I supposed to change to make the footer empty?

Copy link
Contributor

Choose a reason for hiding this comment

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

And by the way, what files am I supposed to change to make the footer empty?

That would be the same footer.md file you already edited, but removing the {{ timestamp }} is fine as well 👍

Do make the changes for the minimal template's files too ( src/template/minimal )

@Parcly-Taxel
Copy link
Contributor Author

@ang-zeyu how about now?

@ang-zeyu
Copy link
Contributor

Ok looks good 👍

Do squash your commits

As for the commit message Default template: fill header and footer, perhaps the subject line should instead address the issue being fixed here. The body could then explain why it occurs, and then the change you made and how it fixes that.
You can browse some of the merged PRs for samples

@Parcly-Taxel
Copy link
Contributor Author

Parcly-Taxel commented Feb 29, 2020

Do squash your commits

@ang-zeyu How do I do that? I've made a pull from master during this pull request, so I can't just rewind all the way, right?

The header, footer and navigation frontmatter for the minimal and
default site templates are empty. This causes inconsistent reloading
when the frontmatter is changed (#1001). Here we populate said
frontmatter.
@ang-zeyu
Copy link
Contributor

@ang-zeyu How do I do that? I've made a pull from master during this pull request, so I can't just rewind all the way, right?

It should be possible to rename your master branch to something else then rebase on top of it, though that will likely mean this PR has to be closed and a new one opened from the renamed branch. You can link back to this PR for some context if that's the case.

This repo practices squash-rebase workflow, so your master branch should track this repo's master;
Your PRs should also hence be from a separate branch, and conflicts should be resolved by rebasing instead of merging. ( you could still merge in the interim though, then rebase once its ready, but its problematic here since your PR is from master )

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.

Inconsistent reloading of site on updating frontmatter
3 participants