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

Generating markdown headers instead of HTML #17

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

DavidMikeSimon
Copy link

This makes the output more idiomatically Markdown-y, and in particular works better with Markdown-parsing tools like markdown-toc

@davidchambers
Copy link
Owner

Idiomatic Markdown is worth pursuing, but I believe this change would break an important feature of Transcribe: clean fragment identifiers. The following links point to the same section, but one is much nicer than the other:

The former is generated by Transcribe; the latter by GitHub.

Could we better interoperate with markdown-toc without sacrificing clean fragment identifiers?

@DavidMikeSimon
Copy link
Author

DavidMikeSimon commented Mar 28, 2018

Two possible approaches:

  • Move the function signature (i.e. everything after the ::) to a line of regular text below the header. Then github's automatic identifier would be alright.

  • Add a span tag around the header text with the generated name

What do you think would be best, if either?

@davidchambers
Copy link
Owner

The first option doesn't suit me because I sometimes rely on case sensitivity. The follow links point to different sections:

The second option sounds promising. The element should be a div rather than a span, though, since headings are block-level elements.

Do Markdown parsers respect Markdown syntax inside HTML elements?

@DavidMikeSimon
Copy link
Author

DavidMikeSimon commented Apr 2, 2018

@davidchambers I was thinking span because it would be an inline-level element inside the block-level markdown header, e.g.:

## <span name="foo">Foo</span>

Which would render to:

<h2><span name="foo">Foo</span></h2>

@DavidMikeSimon
Copy link
Author

@davidchambers Okay, I've made an update. Now the PR does the following:

  • Replace <code> tags with markdown backticks
  • Replace <hX> tags with markdown hashes
  • Still uses HTML <a> tags for links, but moves the name attribute from the <hX> to the <a> tag

I tried the <span> approach above, but GitHub's markdown parser discarded the spans for no reason that I could figure out.

@davidchambers
Copy link
Owner

Thanks, @DavidMikeSimon. I'm eager to put your updated version through its paces. :)

bin/transcribe Outdated
'</a>' +
'</code>' +
'</' + esc(tagName) + '>\n'
markdownHeadingPrefix + ' ' +
Copy link
Owner

Choose a reason for hiding this comment

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

Since markdownHeadingPrefix is referenced just once, let's replace it with its definition.

Copy link
Owner

Choose a reason for hiding this comment

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

We could use '######'.slice(0, options.headingLevel) in place of the Ramda version.

Copy link
Owner

Choose a reason for hiding this comment

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

I spent some time formatting this code. Let's use this:

//. formatSignature :: Options -> String -> Number -> String -> String
var formatSignature = R.curry(function(options, filename, line, signature) {
  return '######'.slice(0, options.headingLevel) + ' ' +
         '<a name="' + esc(signature.split(' :: ')[0]) + '"' +
           ' href="' + esc(options.url.replace('{filename}', filename)
                                      .replace('{line}', line)) + '">' +
           '`' + esc(controlWrapping(signature)) + '`' +
         '</a>\n';
});

@davidchambers
Copy link
Owner

I created sanctuary-js/sanctuary-type-classes@v8.1.1...davidchambers/transcribe to test these changes. I don't see any differences in the rendered output, so this is a strict upgrade. Not only is the text more accessible to markdown-toc, it's more elegant. :)

@DavidMikeSimon
Copy link
Author

@davidchambers Ok, formatted as suggested

Copy link
Owner

@davidchambers davidchambers left a comment

Choose a reason for hiding this comment

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

❤️

@davidchambers davidchambers merged commit 47e481f into davidchambers:master Apr 5, 2018
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.

None yet

2 participants