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

add support to front-matter #2347

Merged
merged 5 commits into from
Sep 15, 2018
Merged

add support to front-matter #2347

merged 5 commits into from
Sep 15, 2018

Conversation

daiyam
Copy link
Contributor

@daiyam daiyam commented Aug 27, 2018

This change adds support of YAML front-matter by:

  • hiding the front-matter in preview
  • skipping the front-matter when looking for note's title

- skip front-matter when looking for note's title
@kazup01 kazup01 added the awaiting review ❇️ Pull request is awaiting a review. label Aug 28, 2018
Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

Please add a test case into /tests/lib/fint-title-test.js like below:

const testCases = [
    ['---\nlayout: test\n---\n # hoge', '# hoge']
  ]

@daiyam
Copy link
Contributor Author

daiyam commented Aug 31, 2018

@sosukesuzuki I've added the test.

@sosukesuzuki
Copy link
Member

@daiyam
I am not familiar with front-matter. 🙇
is the below you expected? or bug?

  • When you start writing front-matter from the second line, it is not hidden.

2018-08-31 17 42 23

@daiyam
Copy link
Contributor Author

daiyam commented Aug 31, 2018

@sosukesuzuki It's because you have an empty line before it.

The front matter must be the first thing in the file and must take the form of valid YAML set between triple-dashed lines.
Jekyll Doc

Here some examples of use

Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

Please check my request

@@ -149,6 +149,7 @@ class Markdown {
})
this.md.use(require('markdown-it-kbd'))
this.md.use(require('markdown-it-admonition'))
this.md.use(require('markdown-it-front-matter'), fm => {})
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need an empty callback? Please remove it if you accidentally put it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No accident, the plugin needs it or it won't work... It used so we can parse the front-matter to extract the properties. But since we don't need to do that, the code block is empty.

Copy link
Member

Choose a reason for hiding this comment

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

Well that's annoying 😄 maybe because the plugin did check if the callback function is provided or not and assume that's there 😄

@@ -1,8 +1,18 @@
const frontMatterRegex = /^\-{3,}/
Copy link
Member

Choose a reason for hiding this comment

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

The front matter must be the first thing in the file and must take the form of valid YAML set between triple-dashed lines.

This regex will also valid for line that started with 4 dashes. You should remove the , there.

Copy link
Contributor Author

@daiyam daiyam Sep 8, 2018

Choose a reason for hiding this comment

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

Yes, you are right.

I've checked markdown-it-front-matter and at first there is Indicated by three or more dashes: ---
It was what I tough but I've check some libraries and all of them use a must be ---.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should stick to the traditional --- 😄

@ZeroX-DG
Copy link
Member

ZeroX-DG commented Sep 8, 2018

I also think if a front matter is invalid then the markdown should display it out. However, with the current behavior, if I type the code bellow, all the content of the note will be hidden.

----
The line bellow has 3 dashes but the line above has 4 dashes and that will hide all the note content.
---

@daiyam
Copy link
Contributor Author

daiyam commented Sep 8, 2018

@ZeroX-DG I've replaced the plugin with my own plugin so that delimiter must be 3 dashes.

}

let line = 0
while (++line < state.lineMax && state.src.substring(state.bMarks[line], state.eMarks[line]) !== '---') {
Copy link
Member

Choose a reason for hiding this comment

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

I still think we should display the markdown if the front-matter syntax is invalid. For example:

---
layout: default
-----
sdasdasdasdasd

Should display:
image
With the current behavior (hide all content) the user will get confuse.

Here is my suggestion for the plugin:

module.exports = function frontMatterPlugin (md) {
  function frontmatter (state, startLine, endLine, silent) {
    if (startLine !== 0 || state.src.substr(startLine, state.eMarks[0]) !== '---') {
      return false
    }

    let line = 0
    let isFrontMatterClosed = false
    while (line < state.lineMax) {
      line++
      if (state.src.substring(state.bMarks[line], state.eMarks[line]) === '---') {
        isFrontMatterClosed = true
        break
      }
    }

    state.line = line + 1

    return isFrontMatterClosed
  }

  md.block.ruler.before('table', 'frontmatter', frontmatter, {
    alt: [ 'paragraph', 'reference', 'blockquote', 'list' ]
  })
}

The the modification above, the front-matter will be displayed out if it's invalid and will be hide only when it's valid.

@daiyam
Copy link
Contributor Author

daiyam commented Sep 9, 2018

@ZeroX-DG yes, it's better to display the invalid front-matter

@ZeroX-DG ZeroX-DG added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Sep 9, 2018
Copy link
Member

@ZeroX-DG ZeroX-DG left a comment

Choose a reason for hiding this comment

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

LGTM

@ZeroX-DG ZeroX-DG added next release (v0.11.10) and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Sep 9, 2018
@Rokt33r Rokt33r merged commit f61fbba into BoostIO:master Sep 15, 2018
@daiyam daiyam deleted the front-matter branch September 15, 2018 07:43
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.

5 participants