attributes-kit icon indicating copy to clipboard operation
attributes-kit copied to clipboard

Markdown Rendering Customization

Open danielgtaylor opened this issue 10 years ago • 11 comments

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?

danielgtaylor avatar Oct 06 '15 16:10 danielgtaylor

@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.

XVincentX avatar Oct 07 '15 11:10 XVincentX

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.

Almad avatar Oct 14 '15 09:10 Almad

I agree with you. :+1:

pksunkara avatar Oct 15 '15 08:10 pksunkara

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

XVincentX avatar Oct 15 '15 11:10 XVincentX

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

Almad avatar Oct 15 '15 13:10 Almad

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.

zdne avatar Oct 19 '15 16:10 zdne

@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.

Almad avatar Oct 19 '15 16:10 Almad

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?

kylef avatar Nov 03 '15 05:11 kylef

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 ...

zdne avatar Nov 03 '15 12:11 zdne

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

pksunkara avatar Nov 03 '15 13:11 pksunkara

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.

Almad avatar Nov 09 '15 15:11 Almad