Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Bug: Phenomic does not render markdown tables #1098

Closed
jfjessup opened this issue Jun 30, 2017 · 9 comments
Closed

Bug: Phenomic does not render markdown tables #1098

jfjessup opened this issue Jun 30, 2017 · 9 comments
Labels

Comments

@jfjessup
Copy link

Phenomic has an issue rendering markdown tables. Please see the snippet of markdown below:

| Left-aligned | Center-aligned | Right-aligned |
| :---         |     :---:      |          ---: |
| left one     | center one     | right one     |
| left two     | center two     | right two     |

It is rendered as this:

Left-aligned
Center-aligned
Right-aligned
left one
center one
right one
left two
center two
right two

The html generated is this:

<table>
  <thead>
    <tr>
      <div>Left-aligned</div>
      <div>Center-aligned</div>
      <div>Right-aligned</div>
    </tr>
  </thead>
  <tbody>
    <tr>
      <div>left one</div>
      <div>center one</div>
      <div>right one</div>
    </tr>
    <tr>
      <div>left two</div>
      <div>center two</div>
      <div>right two</div>
    </tr>
  </tbody>
</table>

The <th> and <td> tags are missing when the markdown syntax tree is rendered by the BodyRenderer.

I think the issue is in the preprocessed markdown syntax tree that is being passed into the browser at run time. The above markdown is passed to the browser as this:

{
  "c": [
    {
      "c": {
        "c": [
          {
            "c": "Left-aligned",
            "p": {
              "align": "left"
            }
          },
          {
            "c": "Center-aligned",
            "p": {
              "align": "center"
            }
          },
          {
            "c": "Right-aligned",
            "p": {
              "align": "right"
            }
          }
        ],
        "t": "tr"
      },
      "t": "thead"
    },
    {
      "c": [
        {
          "c": [
            {
              "c": "left one",
              "p": {
                "align": "left"
              }
            },
            {
              "c": "center one",
              "p": {
                "align": "center"
              }
            },
            {
              "c": "right one",
              "p": {
                "align": "right"
              }
            }
          ],
          "t": "tr"
        },
        {
          "c": [
            {
              "c": "left two",
              "p": {
                "align": "left"
              }
            },
            {
              "c": "center two",
              "p": {
                "align": "center"
              }
            },
            {
              "c": "right two",
              "p": {
                "align": "right"
              }
            }
          ],
          "t": "tr"
        }
      ],
      "t": "tbody"
    }
  ],
  "t": "table"
}

The attribute "t" on the leaf nodes in the syntax tree are not set.

I'm not sure if this is an issue in phenomic or phenomic's use of remark to preprocess markdown files. I'm happy to dig in more and put together a PR, just let me know. The BodyRenderer is a core part of phenomic and phenomic is in active development so I'm holding off for now.

@MoOx
Copy link
Owner

MoOx commented Jul 4, 2017

Thanks for reporting this. I think we have a (probably) too brutal sanitizer
https://github.com/phenomic/phenomic/blob/3847281d1f2601922476c1f9e47f2693cf49ab65/packages/plugin-transform-markdown/src/remark-plugins.js#L38
Sanitizer source: https://github.com/syntax-tree/hast-util-sanitize/blob/master/lib/github.json

Can you check if your stuff and tell me if the sanitizer is the problem?
I already got an issue with it.

FYI

t: tag
p: props
c: children

@MoOx MoOx added the bug label Jul 4, 2017
@jfjessup
Copy link
Author

jfjessup commented Jul 5, 2017

Thanks @MoOx for getting back to me on this issue.

I don't think the sanitizer is the issue here. Logically, phenomic is using the github schema to sanitize the hast syntax tree, but github supports tables so I would assume phenomic wouldn't have an issue. In addition, I pulled the input and output of remark-react's sanitize call and the correct tagNames are there in the input and the output hast trees.

However, when inspecting remark-react I figured out the issue as it's actually an issue with remark-react, not phenomic. The issue is this line. components[name] is only used for <td> and <th> tags, however components[name] is a function expecting props to be passed because it's set here. The code never passes in props so the result is undefined hence why at runtime phenomic is missing the t attribute set here for <td> and <th> elements.

FWIW: once the remark-react issue is fixed phenomic will have a different issue here. The issue will be the component argument will be a single hast node, not a string representing the html tag name. Here's an example:

{
  "p": {
    "style": {
      "textAlign": "center"
    }
  },
  "t": "th"
}

The logic here could be tricky because the component argument now has props to be merged with the props passed into the createComponent function call.

@MoOx
Copy link
Owner

MoOx commented Jul 6, 2017

Thanks for this explanation.
I will take a look and try to fix this problem with the most elegant and reliable way.

@MoOx
Copy link
Owner

MoOx commented Jul 6, 2017

(I think we can simply override td/th components created in remark-react, and just use a string, even if it's a regression somehow... remarkjs/remark-react@c91d4cd

@jfjessup
Copy link
Author

jfjessup commented Jul 8, 2017

Ok. Mind if I put together a PR for that? It could take a few days because I'm on a short vacation right now.

@MoOx
Copy link
Owner

MoOx commented Jul 8, 2017 via email

@owenhoskins
Copy link

Hey @jfjessup & @MoOx: I am curious if you've explored this further? I have a seemingly related issue where the table marked-up ala github doesn't make it into the markdown syntax tree in the browser. Running 1.0.0-alpha.8. Any ideas?

@MoOx
Copy link
Owner

MoOx commented Sep 20, 2017

@owenhoskins I am actually working on this while fixing #817. Except a fix soon (I have a wip that is actually ready).
In short, I am removing all sanitisation process (that are responsible for this kind of issues).

MoOx added a commit that referenced this issue Sep 26, 2017
This plugin now use [unified](http://unifiedjs.github.io) directly instead of [remark](https://github.com/wooorm/remark) so we can mix remark with [rehype](https://github.com/wooorm/rehype) to allow custom markup.
This will open up usage of custom react components when rendering markdown tree with a custom mapping.

Closes [#187](#817) Closes [#1098](#1098)
@MoOx
Copy link
Owner

MoOx commented Sep 26, 2017

Closed by 1bd1d2f

@jfjessup your use case have been added to unit tests!

@MoOx MoOx closed this as completed Sep 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants