problem with github markdown parser

[lowtower]

Hello,

following a short discussion with @froschdesign I report an issue with the github markdown parser regarding pipe symbols.

The pipe sign | in markdown has a special meaning for tables and must therefore be commented out if used otherwise.
As an example, have a look at the table in menu.md.

The final documentation doesn't show the error, but github and maybe other markdown parsers do.

Cheers,
LT.

[weierophinney]

Feel free to open a pull request to make the suggested changes. However, if you do, please see what happens when you run the documentation through mkdocs, to ensure that the final output does not contain the escape characters.

[froschdesign]

@weierophinney
In MkDocs all is fine. The problem is here on Github and with other Markdown parsers. Please look at the original issue report: froschdesign#2

[weierophinney]

@froschdesign I recognize that. My point is if we do any pull requests that add the escape characters, we need to ensure they do not impact generation by mkdocs.

[froschdesign]

@weierophinney
Sorry, you are right. Good point!

[froschdesign]

We should eliminate the tables, because they do not work for mobile / smaller screens.

My suggestions:

• The same description in DocBlock and Documentation.
• A new format, which does not contain any tables.

Example:

#### *public* renderMenu(AbstractContainer $container = null, array $options = []) : string

Renders helper

Renders a HTML 'ul' for the given $container. If $container is not given, the container registered in the helper will be used.

 * `param` AbstractContainer $container [optional] container to create menu from. Default is to use the container retrieved from {@link getContainer()}.
 * `param` array $options [optional] options for controlling rendering

##### Returns

string

Output:

public renderMenu(AbstractContainer $container = null, array $options = []) : string

Renders helper

Renders a HTML 'ul' for the given $container. If $container is not given, the container registered in the helper will be used.

• param AbstractContainer $container [optional] container to create menu from. Default is to use the container retrieved from {@link getContainer()}.
• param array $options [optional] options for controlling rendering

Returns

string

[weierophinney]

That's essentially the direction we went with the ZF2 docs originally; the problem I encountered was difficulty ensuring that all submissions followed the same format.

Additionally, there are definitely cases where tables help. For example, when a config option maps to a method name, and you want to provide a description. That was one area I tried not using tables previously, and found tables were more intuitive, and easier to understand when you were reading.

I understand your argument about mobile devices. However, how often are folks reading the docs on mobile when they need to understand how config keys, methods, etc. work? In most of those cases, I'd expect they'd be reading from whatever machine they're developing on, and, at worst, a tablet.

I'm wary about optimizing away intuitive layouts. 😄

[froschdesign]

@weierophinney
Not only mobile devices have this problem: https://docs.zendframework.com/zend-navigation/helpers/menu/

The table is too large on each device. 😉

[michalbundyra]

@froschdesign @weierophinney Maybe we just need improve our docs templates and add better support for tables - responsive tables?
We can for example wrap columns on smaller devices (screen sizes) or add scroller only for table contents.

[froschdesign]

@webimpress

responsive tables

Already there with Bootstrap's "Responsive tables".

But the problem still stands: the pipe symbols here on Github (and other Markdown parsers). Look at the first table: Menu - wrong rendering for getPartial, setPartial and renderPartial.

[michalbundyra]

@froschdesign I was digging a bit and the only one working solution with github and mkdocs what I have found is:

<code>getPartial() : string&#124;array</code>

I've tried escaping \| and then on github it's ok, but mkdocs generate also escaped character.

I found also suggestion to use double `` and then don't escape | :

``getPartial() : string|array``

but it works only with mkdocs, not with github.

The other solution will be to escape \| and add some script in mkdocs theme (something like: https://github.com/zendframework/zf-mkdoc-theme/blob/master/table_responsive.php) to "unescape" pipes there. But it could be difficult to get 100% right output...

[froschdesign]

@webimpress

…the only one working solution with github and mkdocs…

Or my previous suggestion without tables. 😉

The other solution will be to escape \|…

Which is a quite ugly work and it will always introduce errors.

[michalbundyra]

I had another thought and I would go with escaping pipes with \, so we should have in md files:

`getPartial() : string\|array`

and add script to zend-mkdocs-theme to replace \| to | in table cells.

@froschdesign What do you think?

[froschdesign]

@webimpress
Sorry for the very late response.

…I would go with escaping pipes with \…

For tables I use a Markdown table generator and this generator (and all others I tested) doesn't support the escaping. 😢

[michalbundyra]

@froschdesign I think you misunderstand me.
I ment two things:

1. escaping pipes in markdown (in tables)
2. replacing escaped pipes in table cells after generating html

I've done it and tested in zendframework/zf-mkdoc-theme#30 and #66.

[froschdesign]

@webimpress

I think you misunderstand me.

Right, you mean not the table elements itself, but all other pipes in tables.
At the end this is the same ugly work which will introduce errors.

But I do not see any other solution, so it's okay for me. 👍

[michalbundyra]

@froschdesign

At the end this is the same ugly work which will introduce errors.

Are there any serious errors you can think of?

If we want to have escaped pipeline in table also in html then in markdown it will be escaped 'twice': \\|. My script in mkdoc-theme replace just one \| to | so still in html we will have escaped pipe \| as we expect. I've tested it and all seems to be fine.

Please notice also that I've used preg_match_all and loop to replace pipes only between <td></td> (to avoid greedy regexp behaviour). I can improve it to replace it also in <th> cells or in whole <table>.

[froschdesign]

Are there any serious errors you can think of?

Sure and quite simple:

1. We have to add the extra symbol if we write the documentation.
2. And if we review a PR we have to pay attention to the extra symbol.

Both are sources of error.

[michalbundyra]

@froschdesign Wait, wait ...
You have to add the extra char if you want to create correct md file. Please see here:
https://github.com/zendframework/zend-navigation/blob/master/doc/book/pages.md
pipes are not escaped and table columns are wrong. This is nothing do to with my fix.

This is not serious issue then 😅 Another one? 👿

[froschdesign]

This is nothing do to with my fix.

Don't get me wrong. Your fix is okay, but I see problems writing the documentation. You always have to remember that extra characters have to be set. "Remember" - There I see the error / problem! 😉