Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

Markdown Rendering Customization #123

Open
danielgtaylor opened this issue Oct 6, 2015 · 11 comments
Open

Markdown Rendering Customization #123

danielgtaylor opened this issue Oct 6, 2015 · 11 comments

Comments

@danielgtaylor
Copy link
Contributor

I think it makes sense to ask whether we want to:

  1. Ignore Markdown rendering as a feature (probably bad)
  2. Render Markdown using a library of our choice
  3. Allow custom rendering of Markdown using whatever library a tool is already using

For some tools (e.g. Aglio) it would make a lot of sense to support the third option, since it supports some interesting custom CommonMark extensions. E.g:

screen shot 2015-10-06 at 09 03 44

The way that I can envision this working is by providing a function to the options parameter passed into the render function. That would take a string of Markdown to be rendered and return the rendered result. For example:

const md = new MarkdownIt();

AttributesKit.render(data, element, {
  renderDescription: (input) => md.render(input)
}

What do you think?

@XVincentX
Copy link
Contributor

@danielgtaylor @Baggz

Ignore the Markdown rendering is, of course, out of question. It would give an incomplete experience. On the same time, I agree that the markdown renderer perhaps should be pluggable.

As a simple test, I've prepared #124 , and the thing was quite easy. Another interesting thing is that the fixture diff is minimal (just a simple space). That's a good news.

By the way, I've also checked other markdown implementations, and more or less the function signature is always the same:

render(markdownCode)

So, I would say that your suggestion (a function into the options object) makes sense, and reverting with a default internal implementation eventually.
Thought?

//cc @Almad Who explicitly asked to be looped into the discussion.

@Almad
Copy link
Contributor

Almad commented Oct 14, 2015

Sorry for the later response guys.

@danielgtaylor @XVincentX I think that having DI for rendering definitely makes sense, even because of better testing, however I'd only encourage changing it for 3rd party users.

Internally, for whatever Blueprint/Apiary ecosystem is using, I'd agree on common renderer and common set of switches/options that are used as a default, and used from something like blueprint-markdown-render library.

Therefore, the optional API would be as you've said:

AttributesKit.render(data, element, {
  renderDescription: (input) => md.render(input)
})

however, the "default" usage would be

AttributesKit.render(data, element)

and internally, rendering functions are encouraged to do

import render from 'blueprint-markdown-render';

export default class AttributesKit {
    render(data, element, options) {
        let mdRender = options.renderer;
        if (!mdRender)
             mdRender = render;
    }
}

I'd move discussion around what features should we enable tooling-wide to that repo.

Does that make sense?

//cc @kylef and @zdne as well, as I think we should agree on this accross the board.

@pksunkara
Copy link

I agree with you. 👍

@XVincentX
Copy link
Contributor

@Almad What's the status of blueprint-markdown-renderer? Do we have it already, are we crafting it?

@Almad
Copy link
Contributor

Almad commented Oct 15, 2015

No, I am proposing having it, and waiting for you guys to support the idea :)

@zdne
Copy link

zdne commented Oct 19, 2015

I do not think there should be such a thing as blueprint-markdown-renderer – it is an engineering overkill that haven't be needed in the past few years. Furthermore it will put more burden on the parser. If we bake a C/C++ renderer into drafter and then create its emscripten version for it we are essentially loosing here. I do not see any benefits here.

Besides I have already said it many times but I want to pivot blueprint away from being a Markdown or GFM Markdown in the favor of plain text.

@Almad
Copy link
Contributor

Almad commented Oct 19, 2015

@zdne I think it have been, as it resulted in inconsistencies in our rendering toolchain.

I am not talking about baking it into drafter/C++ toolchain, I am talking about having agreed default on top of it.

Which means that the Markdown pivot is irrelevant, as I am talking about rendering Markdown blocks only, not the blueprint as a whole.

@kylef
Copy link
Member

kylef commented Nov 3, 2015

I agree that we should try and align the markdown parsers used in our own products as @Almad suggests. Settling on something like CommonMark which has a clearly defined specification would be the best case.

Third party tools should have the capability of changing the renderer. If a user would like to explicitly change the parser to add extensions I believe this should be allowed. However, only if the user explicitly enables these features. This allows the default experience of a blueprint to consistently render on all tools unless a user has explicitly changed this behaviour.

What do you think?

@zdne
Copy link

zdne commented Nov 3, 2015

When it comes to CommonMark – we need to conduct a research on whereas it supports source maps before we can start thinking about using it in the Markdown Parser this research is long overdue and maybe we should schedule @kylef ...

@pksunkara
Copy link

IIRC hoedown has source maps support for CommonMark. But yes, I think we should schedule it soon.

@Almad
Copy link
Contributor

Almad commented Nov 9, 2015

I am lagging, because as I've looked into it deeper, I feel that I need to resolve few other issues:

  • Escaping is different in CommonMark & markdown-it and different than what we currently have in toolchain/documentation, and I think this shouldn't be changed lightly
  • markdown-it offers some good plugins, yet uses token stream while CommonMark uses AST

Will look into it deeper this week.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants