-
Notifications
You must be signed in to change notification settings - Fork 604
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
[api-documenter] markdown: fixes newline in table code blocks #1234
Conversation
@acchou FYI |
The change looks good to me for preserving line breaks. Though it would be nicer to also preserve indentation somehow. |
common/changes/@microsoft/api-documenter/fix-newline-in-table-code-block_2019-04-15-10-59.json
Outdated
Show resolved
Hide resolved
@@ -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>')); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @acchou, useful feedback!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@skaapgif Thanks for fixing this! |
[reopening PR -- apparently I clicked the wrong button] |
Fixes the rendering of newlines inside table code blocks.
Examples:
({ children }: {
<p/>
children: string;<p/>
}) => boolean({ children }: {
children: string;
}) => boolean
({ children }: {
children: string;
}) => boolean
({ children }: {
children: string;
}) => boolean
({ children }: {
children: string;
}) => boolean
Source code for above table: https://gist.githubusercontent.com/skaapgif/d0df63fa306f578363093d0426ebb20b/raw/6bb7eea899c8ca141065511c9f7d6c293324c353/code%2520in%2520markdown%2520table%2520with%2520line%2520breaks.md