Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[api-documenter] markdown: fixes newline in table code blocks #1234

Merged
merged 4 commits into from
Apr 17, 2019

Conversation

rudolf
Copy link
Contributor

@rudolf rudolf commented Apr 15, 2019

Fixes the rendering of newlines inside table code blocks.

Examples:

Property Type Description
Context ({ children }: {<p/> children: string;<p/> }) => boolean Behaviour of #1183
Context ({ children }: {

children: string;

}) => boolean

Attempt #1 don't escape paragraph tags
Context

({ children }: {

children: string;

}) => boolean

Attempt #2 embed code in paragraph tags
Context ({ children }: {
children: string;
}) => boolean
Attempt #3 use br tags inside code tags
Context ({ children }: {
children: string;
}) => boolean
Best solution: end code tag + br tag

Source code for above table: https://gist.githubusercontent.com/skaapgif/d0df63fa306f578363093d0426ebb20b/raw/6bb7eea899c8ca141065511c9f7d6c293324c353/code%2520in%2520markdown%2520table%2520with%2520line%2520breaks.md

@octogonz
Copy link
Collaborator

@acchou FYI

@acchou
Copy link
Contributor

acchou commented Apr 15, 2019

The change looks good to me for preserving line breaks. Though it would be nicer to also preserve indentation somehow.

@@ -115,7 +115,7 @@ export class MarkdownEmitter {
if (context.insideTable) {
const code: string = this.getTableEscapedText(docCodeSpan.code);
const parts: string[] = code.split(/\r?\n/g);
writer.write(parts.join('`<p/>`'));
writer.write(parts.join('</code><br/><code>'));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that you've found the best possible solution, but I'll point out that it has the downside that each line of code will have a separate styling rectangle. It looks okay on GitHub, but results may vary with other Markdown renders.

I've never liked the pipes-and-dashes table construct. It has many limitations, and the syntax isn't particularly easy to write. If I remember right, CommonMark chose not to standardize the pipes-and-dashes table after a very long discussion about it. My general philosophy is that while Markdown saves a lot of typing for basic formatting problems, we should defer to using HTML for any complex nesting structures. (This is the philosophy that I'm advocating for TSDoc.)

Unfortunately, as soon as we introduce an HTML table tag, it turns off Markdown entirely:

<table><tr><td>
`code`
**bold**
</td></tr></table>

gets rendered as:

`code` **bold**

Long term, I'd like to add an api-documenter html command line that generates (style-neutral) HTML instead of Markdown. HTML is much easier to generate, whereas Markdown was intended for an interactive human editing experience. Markdown is very difficult for a machine to generate correctly as we see in this PR. The main reason we chose Markdown initially was to support a custom documentation engine that doesn't accept HTML input. This is a fairly common situation, though, so api-documenter should certainly continue to support Markdown forever, alongside any other output types we may add.

Another idea I've considered is finding an existing documentation generator with very professional templates, and writing an adapter to drive it using .api.json files. I wouldn't want to bake too much fancy rendering into api-documenter, because its goal is to be complete but simple and readable, so people can use its source code as a guide when writing their own custom solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad to hear that Markdown will be supported going forward. There are still good reasons to support it fully:

  • markdown allows documentation to be viewed in GitHub repositories naturally. You can link to documentation from a README and it will render nicely.
  • markdown is common as an input format for static site generators commonly used for documentation. For example, docusaurus can consume api-documenter output and render html with the same style as other pages it generates. This makes for a much nicer documentation site.

If tables turn out to be a continuing source of trouble, maybe there's a way to generate reasonable output without tables. In any case, while I agree that html is better specified and would make it easier to generate, I'd vote for keeping markdown for its interoperability.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @acchou, useful feedback!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw do you have an example of docusaurus output? Maybe we should mention this option in the docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on my site right now, I will post a link here when it's published.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I'm also curious whether you will try to implement a navigation tree for your docs. Markdown by itself doesn't provide any facility for this, but we could easily write the tree into a JSON file similar to what we do for DocFX. In that case you might ask to disable the breadcrumb entirely.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A navigation tree might be nice, but I don't consider it a priority until it's clear the existing breadcrumbs are insufficient. Plus I plan on adding search to the site so that should alleviate some of the need.

Another way to mitigate the demand for navigation would be to reorganize the output so there aren't so many files generated. It would be nice if every class or module was a single file. There are many pages that just have a short description and little else.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I added your link as an example for the Generating API docs article

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks!

Also, to drive home the "Home" link issue, note that that node-core-library documentation's breadcrumb for "Home" is a 404.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acchou thanks for reporting this!

The missing extension works okay on GitHub pages but I didn't realize that it was broken in the GitHub markdown viewer. Here's a PR to fix it: #1251

@octogonz
Copy link
Collaborator

octogonz commented Apr 15, 2019

@skaapgif Thanks for fixing this!

@octogonz octogonz closed this Apr 15, 2019
@octogonz octogonz reopened this Apr 15, 2019
@octogonz
Copy link
Collaborator

[reopening PR -- apparently I clicked the wrong button]

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

Successfully merging this pull request may close these issues.

3 participants