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 omitFrontmatter attribute to include #1448

Merged
merged 7 commits into from
Jan 20, 2021

Conversation

ryoarmanda
Copy link
Contributor

@ryoarmanda ryoarmanda commented Jan 16, 2021

What is the purpose of this pull request?

  • Feature addition or enhancement

Resolves #720

Overview of changes:

  • Added omitFrontmatter attribute to <include> so that the frontmatter data of the included page will not be present to override the "parent" page's frontmatter.
  • Added new property processingOptions to Context to contain options that are relevant for processing, such as discarding front matter data.
  • Added two tests to ensure include processing routine preserves/discards the frontmatter data depending on whether the omitFrontmatter attribute is present.

Anything you'd like to highlight / discuss:

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)
Add omitFrontmatter attribute to include

Including a file combines the front matter of the file with the main
page. When authors wish to include the full content of the file but not
the front matter, they have to make their whole content a segment as a
work around, making the content less concise.

Let's add a new attribute to indicate whether the front matter data of
the included file should be omitted.


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes
  • Pinged someone for a review!

@ryoarmanda
Copy link
Contributor Author

@ang-zeyu , I was trying to check a test that I have written for this and I noticed that when I run npm run test on the core package, it only reports 4 test suites/files, Jest didn't account for includePanelProcessorTest.js, and I might be wrong but I feel like Jest didn't run any tests in that file. Is that intended? I'm not sure how to check my tests now.

@ang-zeyu
Copy link
Contributor

@ang-zeyu , I was trying to check a test that I have written for this and I noticed that when I run npm run test on the core package, it only reports 4 test suites/files, Jest didn't account for includePanelProcessorTest.js, and I might be wrong but I feel like Jest didn't run any tests in that file. Is that intended? I'm not sure how to check my tests now.

Dosen't sound right

when I run npm run test on the core package

is this run on just the core package? or at the root? (as vue-components package has 4 test suites)
I'm seeing 5 tests running on your github action logs as well

if not, try running the jest includePanelProcessorTest.js manually and see what happens

I'm not sure how to check my tests now.

You can create a quick failing test to make sure (e.g. expect(true).toBe(false))

@ryoarmanda
Copy link
Contributor Author

ryoarmanda commented Jan 18, 2021

Tried testing within the core package, still only 4 test suites:

image

Checked the github action logs as well, only 4 too

Perhaps because naming of the test file? It is includePanelProcessorTest.js. I tried renaming it to includePanelProcessor.test.js and Jest detects it, though most of the existing tests in that file failed. 😅

(Edit: Looks like the tests are outdated, the way the includes are structured is different. I might need to rewrite the expected html of the tests...)

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Jan 18, 2021

Tried testing within the core package, still only 4 test suites:

image

Checked the github action logs as well, only 4 too

Perhaps because naming of the test file? It is includePanelProcessorTest.js. I tried renaming it to includePanelProcessor.test.js and Jest detects it, though most of the existing tests in that file failed. 😅

(Edit: Looks like the tests are outdated, the way the includes are structured is different. I might need to rewrite the expected html of the tests...)

Yikes, nice catch, I renamed it at one point

I'll fix it in a separate PR shortly, unless you've already started

(Edit: Looks like the tests are outdated, the way the includes are structured is different. I might need to rewrite the expected html of the tests...)

If you're fixing it, the diffs are expected and are a result of the combining markdown processing and <include /> stages. (we used to have nunjucks -> html (just to process <include />s) -> markdown -> html)

@ryoarmanda
Copy link
Contributor Author

ryoarmanda commented Jan 18, 2021

If you're fixing it, the diffs are expected and are a result of the combining markdown processing and <include /> stages. (we used to have nunjucks -> html (just to process <include />s) -> markdown -> html)

I see. I'm on it, will create another PR soon. 👍

@ryoarmanda ryoarmanda changed the title [WIP] Add omitFrontmatter attribute to include Add omitFrontmatter attribute to include Jan 19, 2021
@ryoarmanda
Copy link
Contributor Author

@ang-zeyu I am now all done with this PR, can you help take a look on the code and the tests?

@ang-zeyu ang-zeyu merged commit 1b52b6f into MarkBind:master Jan 20, 2021
@ang-zeyu ang-zeyu added this to the v3.0 milestone Jan 20, 2021
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.

include: give a way to omit frontmatter
2 participants