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 renderMarkdown to ICompiler #16

Merged
merged 3 commits into from
Mar 15, 2017
Merged

add renderMarkdown to ICompiler #16

merged 3 commits into from
Mar 15, 2017

Conversation

giladgray
Copy link
Contributor

KssPlugin markdownifies documentation + markup for syntax highlighting

Gilad Gray added 2 commits March 9, 2017 18:41
just the markdown piece, useful for plugins that don't need full `renderBlock`
@giladgray
Copy link
Contributor Author

@adidahiya mind reviewing this as @themadcreator is out today?

@giladgray
Copy link
Contributor Author

though i suppose it could wait till monday

@giladgray giladgray requested a review from themadcreator March 15, 2017 00:27
Copy link
Contributor

@themadcreator themadcreator left a comment

Choose a reason for hiding this comment

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

Please don't inject map's this context

@@ -57,18 +65,19 @@ export class KssPlugin implements IPlugin<IKssPluginData> {
}
}

function convertSection(section: kss.ISection): IKssExample {
function convertSection(this: ICompiler, section: kss.ISection): IKssExample {
Copy link
Contributor

Choose a reason for hiding this comment

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

using map's this context in this fashion is pretty hacky. I know it's more performant than using a lambda in the map above, but just one more arg and this doesn't work.

I think i'd prefer to just use an explicit argument and a lambda in the map.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it's so damn clever 👩‍🔬

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, and kudos. But simple and clear is better, IMO

markup: section.markup() || "",
modifiers: section.modifiers().map(convertModifier),
markupHtml: this.renderMarkdown(`\`\`\`html\n${section.markup() || ""}\n\`\`\``),
modifiers: section.modifiers().map(convertModifier, this),
Copy link
Contributor

Choose a reason for hiding this comment

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

And again =(

/**
* Render a string of markdown to HTML, using the options from `Documentalist`.
*/
renderMarkdown: (markdown: string) => string;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@giladgray giladgray force-pushed the gg/render-markdown branch from 3ce09f6 to de6d198 Compare March 15, 2017 18:43
@blueprint-bot
Copy link

use lambda instead of this context

Preview: docs

@giladgray giladgray merged commit 50f7550 into master Mar 15, 2017
@giladgray giladgray deleted the gg/render-markdown branch March 15, 2017 20:36
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.

3 participants