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 optional includes for page fragments #517

Merged
merged 2 commits into from Jan 3, 2019
Merged

Support optional includes for page fragments #517

merged 2 commits into from Jan 3, 2019

Conversation

Xenonym
Copy link
Contributor

@Xenonym Xenonym commented Jan 1, 2019

What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Enhancement to an existing feature

Resolves #497.

What is the rationale for this request?
Currently, we allow optional inclusion of whole documents with the optional tag eg.
<include src="doc.md" optional />. This PR extends this functionality to page fragments as well eg.
<include src="doc.md#segment" optional /> will be included only if #segment exists.

What changes did you make? (Give an overview)
When a page fragment that is included does not exist, we check first if the optional tag is used. If so, we include an empty string in place, leading to an empty <div>. If not, an error is emitted that the page fragment included does not exist.

Provide some example code that this change will affect:

<include src="doc.md#exists" optional />
<include src="doc.md#doesNotExist" optional />
<include src="doc.md#doesNotExist" />

Is there anything you'd like reviewers to focus on?
In previous versions, inclusion of nonexistent page fragments lead to a <div> with the string null being included instead. Since this behavior is likely to be confusing with the addition of optional, I have chosen to change this so that an error is emitted when an included page fragment does not exist and optional was not used. I have also added tests to verify this new behavior alongside those that verify the behavior of optional with page fragments.

Testing instructions:

  1. Include a page fragment with optional eg. <include src="doc.md#exists" optional />. The content of the page fragment should be included.
  2. Include a nonexistent page fragment with optional eg.
    <include src="doc.md#doesNotExist" optional />. There should not be an error and the page should render as normal.
  3. Include a nonexistent page fragment normally eg. <include src="doc.md#doesNotExist" />. An error should be emitted.

Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Ok, just a minor nit in error message:

src/lib/markbind/src/parser.js Outdated Show resolved Hide resolved
@yamgent yamgent added this to the v1.15.3 milestone Jan 2, 2019
@yamgent yamgent merged commit c49f86e into MarkBind:master Jan 3, 2019
@Xenonym Xenonym deleted the include-optional-fragment branch January 3, 2019 02:51
@damithc
Copy link
Contributor

damithc commented Jan 3, 2019

This probably needs a slight update to docs?

@yamgent
Copy link
Member

yamgent commented Jan 3, 2019

This probably needs a slight update to docs?

Hmm, it didn't cross my mind for this feature, as we do not explicitly mention that other attributes such as inline and trim works for fragments as well in the current documentation, because it seems to be implied.

Although I agree that a minor mention might make it clearer and more obvious for the author. Maybe such a mention will work?

diff --git a/docs/userGuide/reusingContents.md b/docs/userGuide/reusingContents.md
index e9e957a..15dea5b 100644
--- a/docs/userGuide/reusingContents.md
+++ b/docs/userGuide/reusingContents.md
@@ -186,14 +186,14 @@ Attributes:
   <include src="frament.md" trim />
   \```
 
-**You can `<include>` a fragment of a file** by specifying the `#fragment-id` at the end of the `src` attribute value, provided the fragment is wrapped in a `<div>`/`<span>`/`<seg>` tag with the matching `id`.
+**You can `<include>` a fragment of a file** by specifying the `#fragment-id` at the end of the `src` attribute value, provided the fragment is wrapped in a `<div>`/`<span>`/`<seg>` tag with the matching `id`. You can use the attributes for documents (such as `inline` and `optional`) with fragments as well.
 
 <div class="indented">
 
 {{ icon_example }} Including a fragment from a file:
 \```html
 Some text
-<include src="docs/tips.md#tip-1" />
+<include src="docs/tips.md#tip-1" trim />
 Some other text
 \```

@damithc
Copy link
Contributor

damithc commented Jan 3, 2019

Yes, it is awkward to mention it in the current doc. The problem is, fragment inclusion is mention after the attributes. Let's leave it be.

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.

Extend 'optional include' functionality to page fragments
3 participants