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

Pass template context to the Markdown engine #643

Merged
merged 5 commits into from
Jan 23, 2020

Conversation

blacksquaresa
Copy link

Good morning,

The project I am using eleventy.js in makes extensive use of markdown-it plugins to customise the behaviour, and add special handlers. It would be extremely useful for those plugins to have access to the context of the template they are processing. Markdown-it allows an external context to be passed to its rendermethod, so all that is missing is for eleventy to actually pass that context. Turns out, that context is available right there - it just needs to be used.

So this is a very small pull request, with a minor change, and two tests to check the functionality. I have added the tests in their own file - I wasn't sure if that was correct, or if you would want me to add them to an existing file.

Thanks

@@ -79,9 +79,8 @@ class Markdown extends TemplateEngine {
return str;
};
} else {
return function() {
// throw away data if preTemplateEngine is falsy
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you drop the comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can tell the comment is not relevant anymore

Copy link
Author

Choose a reason for hiding this comment

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

As @MadeByMike mentioned, the comment says, "throw away data", but we're not throwing away the data any more.

Copy link
Contributor

@MadeByMike MadeByMike left a comment

Choose a reason for hiding this comment

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

@blacksquaresa, this looks pretty good to me! My only suggestion is that the deeply nested if statements could probably be reorganised into a more readable structure. But that's not on you :)

@@ -79,9 +79,8 @@ class Markdown extends TemplateEngine {
return str;
};
} else {
return function() {
// throw away data if preTemplateEngine is falsy
Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can tell the comment is not relevant anymore

@zachleat
Copy link
Member

I like this idea. Can you give an example of where this data might be used in markdown-it? I’m not aware of markdown using data like this

@zachleat zachleat added the open-question Requires additional information before we can proceed. label Aug 13, 2019
@blacksquaresa
Copy link
Author

I like this idea. Can you give an example of where this data might be used in markdown-it? I’m not aware of markdown using data like this

In my case, I use a markdown-it plugin (markdown-it-replace-link) to resolve links in my markdown pages. The plugin calls a callback I provide, and passes it the context it gets from markdown-it.

Without this change, I have to resolve my links as absolute urls (/path/to/target/index.html), because while I know the target, I don't know which page the current link came from.

With this change, I can resolve the urls into relative links (../../target/index.html), because I have access to both the target and the url of the current page. This is important because my site needs to be able to be installed at an unknown position in the hierarchy on a webserver (could be the root, could be under 'site/', etc).

@blacksquaresa
Copy link
Author

@zachleat , did you see my answer to your open question? Do you need more information?

@zachleat
Copy link
Member

Hey @blacksquaresa sorry about the delay, I’m working my way through PRs now and here I am!

Can you make a test that shows how the link would change?

image

Specifically, can you add a test checks the output from result to show how the link might change here?

@zachleat zachleat added in-progress and removed open-question Requires additional information before we can proceed. labels Nov 27, 2019
@blacksquaresa
Copy link
Author

Specifically, can you add a test checks the output from result to show how the link might change here?

I have updated the mock plugin to modify the link using the data object being passed in, then check that the correct HTML is output, including the modified link. @zachleat , Is this what you were looking for?

@zachleat
Copy link
Member

Perfect, thank you!

@zachleat zachleat merged commit 0c130eb into 11ty:master Jan 23, 2020
@zachleat zachleat added this to the Next Minor Version milestone Jan 23, 2020
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.

4 participants