christianalfoni / markdown-to-react-components

Convert markdown into react components
MIT License
132 stars 16 forks source link

Blockquote generates double `p` #8

Closed bebraw closed 8 years ago

bebraw commented 8 years ago

Demo:

var React = require('react');
var ReactDOM = require('react-dom/server');
var MTRC = require('markdown-to-react-components');

console.log(ReactDOM.renderToStaticMarkup(
  React.createElement('div', {}, MTRC('> Demo').tree))
);
{
  "name": "mtrc-demo",
  "version": "1.0.0",
  "description": "",
  "main": "demo.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "MIT",
  "dependencies": {
    "markdown-to-react-components": "^0.1.4",
    "react": "^0.14.3",
    "react-dom": "^0.14.3"
  }
}

This yields:

Warning: Each child in an array or iterator should have a unique "key" prop. Check the top-level render call using <div>. See https://fb.me/react-warning-keys for more information.
Warning: validateDOMNesting(...): <p> cannot appear as a descendant of <p>. See p > ... > p.
<div><blockquote><p><p>Demo</p></p></blockquote></div>

There shouldn't be a p inside p.

christianalfoni commented 8 years ago

Hm, this only happens when the blockquote is the first thing in the markdown string. So that is handled differently by the "marked" converter... which I suppose it should not.

You agree it is a "marked" bug?

bebraw commented 8 years ago

I'm not sure. Demo:

const marked = require('marked');

console.log(marked('> Demo'));

gives

<blockquote>
<p>Demo</p>
</blockquote>

which looks ok to me.

The input is the same. It's possible I'm missing some nuance here. MTRC goes through marked parser after all so maybe the demo needs to be expanded.

christianalfoni commented 8 years ago

Hm, I am thinking that maybe we should use the lexer instead: https://github.com/chjj/marked

bebraw commented 8 years ago

Yeah. That's a possibility. I know markdown-it is an alternative.

mdast-react actually does more or less the same as MTRC but it seems unmaintained at the moment. Spectacle uses the version I linked to.

christianalfoni commented 8 years ago

Thanks for the info Juho :-) I need a weekend mini project, looks fun using the lexer to build the components instead. Also seems a lot safer, as I am doing some crazy trickery with shared state between the constructors.

By the end of the weekend I should have something ;-)

bebraw commented 8 years ago

Ok, cool. One interesting option would be simply to fork mdast-react by jjt and then move from there based on your needs. That could save significant effort.

I also love the fact that there are nice plugins, like remark-toc, for mdast.

christianalfoni commented 8 years ago

Hm, does it have any missing features compared to this project? I only built this because I could not find any markdown react projects that actually built react components :-)

bebraw commented 8 years ago

It looks somewhat feature complete to me. The only part I'm a little unsure of is highlighting. I guess you'll know better by trying it out. :+1:

christianalfoni commented 8 years ago

Okay, let me check it out this weekend. It looks like fun to play around with the lexer thingy. Typical "beer project" :-) But if the other one is feature complete I do not see any reason to have "yet another" markdown to react components project. But good point on highlighting, that is an important feature.

wooorm commented 8 years ago

A note about highlighting: remark combined with remark-html has support for injecting HTML by plug-ins, e.g., mdast-highlight.js, however, when injecting into a (virtual) DOM on the client it’s often a bad (unsafe) idea due to having to depend on dangerouslySetInnerHTML.

It would be better to pass in custom components, e.g., a quick search gave me react-highlight. Which, should be possible to use in mdast-react (and otherwise we should build it!)