feat(markdown): add ids to headers #713

glebec · 2018-09-21T19:10:57Z

What type of PR is this? (check all applicable)

Refactor
Feature
Bug Fix
Documentation Update

Description

Enables the with_toc_data option for the Redcarpet markdown renderer. This adds id attributes to the markdown headers, allowing for hash links to specific page content. This is useful both for linking and for generating tables of content (ToCs).

Input:

# Title

Text

## More

Etc.

Rendered (before):

<h1>Title</h1>
<p>Text</p>
<h2>More</h2>
<p>Etc.</p>

Rendered (after):

<h1 id="title">Title</h1>
<p>Text</p>
<h2 id="more">More</h2>
<p>Etc.</p>

NB, I don't know Ruby so you should definitely sanity-check the code and make sure it makes sense w.r.t. how these classes & functions are intended to work.

Related Tickets & Documents

Might be considered to close #183. However, you might also want a feature similar to GitHub's markdown renderer, which makes it clear that headers have anchors via a link or hover icon.

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

docs.dev.to
readme
no documentation needed

bennypowers · 2018-09-27T13:46:47Z

I'd like to try my hand at getting those tests passing. @glebec are you comfortable adding me as a contributor to your fork?

glebec · 2018-09-27T13:48:21Z

Please do – I was probably going to post that anyone who wanted to take a crack at this branch should feel free, as I don’t have a strong need for this feature currently myself.

app/labor/markdown_parser.rb

Zhao-Andy · 2018-09-27T17:34:22Z

Hey @bennypowers and @glebec, this is very late notice (apologies), but I have a PR for this issue for this in our old repo that is mostly done. I'll bring that PR over since I've worked out most of the bugs and kinks that come along the way.

If you'd like, you can keep working on this PR, but I think it makes sense for us to take a look at both PRs and see which one should be merged into the other. Sorry for letting the crossed wires happen!

glebec · 2018-09-27T17:39:17Z

@Zhao-Andy no worries, I haven't spent any serious time or thought on this PR – nor do I know this codebase (or even language) well. So I'm going to assume a priori that your branch is probably going to be farther along. We can certainly review that, of course.

bennypowers · 2018-09-27T17:39:49Z

@Zhao-Andy in that case I'll finish up with my current test and fire it off for comparisson. Thanks!

bennypowers · 2018-09-30T10:34:07Z

I got bored so I fixed the tests ;)

glebec · 2018-09-30T18:19:38Z

spec/labor/markdown_parser_spec.rb

@@ -54,7 +54,7 @@ def generate_and_parse_markdown(raw_markdown)
    it "does not generated nested link tags" do
      nested_links = generate_and_parse_markdown("[[](http://b)](http://a)")
      nested_links = Nokogiri::HTML(nested_links).at("p").inner_html
-      expect(nested_links).to eq('[<a href="http://b" rel="noopener noreferrer"></a>](<a href="http://a" rel="noopener noreferrer">http://a</a>)') # rubocop:disable Metrics/LineLength
+      expect(nested_links).to eq('[<a href="http://b"></a>](<a href="http://a">http://a</a>)')


~~I'm curious why this PR's changes invalidated the former test and mean that the rels had to be taken out. I personally don't have any stake in those rels existing, just doing some due diligence.~~

Nevermind – I was looking at only 81d998e's changes. I failed to notice that those changes undid some earlier changes in this same PR.

glebec · 2018-09-30T18:21:00Z

spec/models/comment_spec.rb

@@ -44,7 +44,7 @@
    end

    it "converts body_markdown to proper processed_html" do
-      expect(comment.processed_html.include?('<h1 id="hello-hy-hey-hey">')).to eq(true)
+      expect(comment.processed_html.include?('<h1 id="hello">')).to eq(true)


Out of curiosity, why do ids behave differently now? If this test didn't make sense before, why did it previously pass? Is there any observable behavior change that might break earlier content?

Just doing due diligence, not saying how it ought to be.

The original test tested against <h1>, which would fail if the markdown text was

# hello hy hey hey

This change just includes the anchor id in an otherwise unrelated test.

glebec · 2018-09-30T18:25:19Z

@bennypowers is this ready to have the WIP prefix removed (i.e. ready for this to be reviewed again)?

bennypowers · 2018-10-01T16:35:28Z

@glebec yeah afaict it's ready to roll

bennypowers · 2018-10-01T17:34:27Z

@Zhao-Andy interesting points for comparison: testing for emoji in headers.

Zhao-Andy · 2018-10-11T18:59:17Z

Hey @glebec, finally getting around to this now; sorry about that. Mind if I push up to your branch directly? Seems to be the easiest way to keep the history clean.

glebec · 2018-10-11T19:08:30Z

I'm fine with that, will add you as a collaborator shortly. EDIT: ok, you should have received an invite to collab at https://github.com/glebec/dev.to.

Zhao-Andy · 2018-10-11T20:26:26Z

Just made a push. I made a couple of changes:

I'd rather use an a tag with the name attribute instead of the built in with_toc_data option even though it's more verbose. This keeps us safe from people using headers with ids that match ours, and prevents them from using IDs that we don't want. I'm not sure how big of a security issue is, but seeing as GitHub uses a tags with names instead of headers with ids, it's probably safe to follow suit.
I added some styling that should make the headers properly jump to the header, as opposed to the paragraph. This happens because we have a sticky navbar on the top, and the native HTML jumps to the top of the header (or a tag in this case).

In the future, we might add an icon to the left side of the headers much like GitHub does in READMEs.

I tried that in my previous PR but it was such a pain to get the styling correct for both articles and comments. Maybe in the future though :)

bennypowers · 2018-10-11T20:59:12Z

i'd suggest that emoji get stripped out of the anchor name rather than converted to unicode address

Zhao-Andy · 2018-10-12T19:02:14Z

spec/support/fixtures/approvals/user_preview_article_body.approved.html

+</h1>
+
+<h1>
+  <a name="%F0%9F%98%8E-emoji-heading" href="#%F0%9F%98%8E-emoji-heading" class="anchor" id="%F0%9F%98%8E-emoji-heading">


Not sure why this looks like this file, but it works fine in development.

Zhao-Andy · 2018-10-12T19:02:57Z

Here it is in action:

bennypowers · 2018-10-13T16:31:20Z

Haven't reviewed the code but for the third example it should probably strip the trailing -

bennypowers · 2018-11-19T08:22:07Z

👋

Zhao-Andy · 2018-11-19T17:29:06Z

Hey @bennypowers, thanks for the bump. We'll get to this soon. 🙈

feat(markdown): add anchors for headers

d543b91

glebec changed the title ~~feat(markdown): add ids to headers~~ WIP feat(markdown): add ids to headers Sep 22, 2018

maestromac reviewed Sep 27, 2018

View reviewed changes

app/labor/markdown_parser.rb Outdated Show resolved Hide resolved

bennypowers force-pushed the feat/markdown-anchors branch from b973c28 to 6ead028 Compare September 27, 2018 17:58

Fix markdown anchors impl and spec

cb376bc

bennypowers force-pushed the feat/markdown-anchors branch from 6ead028 to cb376bc Compare September 27, 2018 18:00

Fixes tests for markdown anchors

81d998e

glebec commented Sep 30, 2018

View reviewed changes

glebec changed the title ~~WIP feat(markdown): add ids to headers~~ feat(markdown): add ids to headers Oct 1, 2018

weekly-digest bot mentioned this pull request Oct 5, 2018

Weekly Digest (28 September, 2018 - 5 October, 2018) #834

Closed

Merge branch 'master' into feat/markdown-anchors

9e3df51

Zhao-Andy added 2 commits October 11, 2018 16:06

Merge branch 'master' into feat/markdown-anchors

6f99a3d

Use name for anchors instead of IDs

91ef9eb

Fix approval file

6cecb4a

Zhao-Andy force-pushed the feat/markdown-anchors branch from 29470b2 to 6cecb4a Compare October 12, 2018 14:17

Zhao-Andy added 2 commits October 12, 2018 13:23

Merge branch 'master' into feat/markdown-anchors

02672ac

Remove emojis in links and hyphenate only spaces

5354261

Zhao-Andy force-pushed the feat/markdown-anchors branch from 1f09ca1 to 5354261 Compare October 12, 2018 19:01

Zhao-Andy reviewed Oct 12, 2018

View reviewed changes

maestromac mentioned this pull request Oct 18, 2018

Allow jumping/linking to headings with a URL hash #178

Closed

bennypowers mentioned this pull request Nov 19, 2018

Add headings to header tags #1003

Merged

7 tasks

benhalpern added 2 commits November 20, 2018 14:49

Merge branch 'master' into feat/markdown-anchors

d5d38c7

Merge branch 'master' into feat/markdown-anchors

ec57490

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Nov 20, 2018

Merge branch 'master' into feat/markdown-anchors

e1f1853

benhalpern approved these changes Nov 20, 2018

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Nov 20, 2018

benhalpern merged commit 1e2749b into forem:master Nov 20, 2018

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Nov 20, 2018

glebec deleted the feat/markdown-anchors branch November 21, 2018 03:50

glebec restored the feat/markdown-anchors branch November 21, 2018 03:50

glebec deleted the feat/markdown-anchors branch November 21, 2018 03:50

glebec restored the feat/markdown-anchors branch November 21, 2018 03:51

feat(markdown): add ids to headers #713

feat(markdown): add ids to headers #713

Uh oh!

Conversation

glebec commented Sep 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Added to documentation?

Uh oh!

bennypowers commented Sep 27, 2018

Uh oh!

glebec commented Sep 27, 2018

Uh oh!

Uh oh!

Zhao-Andy commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glebec commented Sep 27, 2018

Uh oh!

bennypowers commented Sep 27, 2018

Uh oh!

bennypowers commented Sep 30, 2018

Uh oh!

glebec Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glebec Sep 30, 2018

Choose a reason for hiding this comment

Uh oh!

glebec Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bennypowers Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

glebec commented Sep 30, 2018

Uh oh!

bennypowers commented Oct 1, 2018

Uh oh!

bennypowers commented Oct 1, 2018

Uh oh!

Zhao-Andy commented Oct 11, 2018

Uh oh!

glebec commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zhao-Andy commented Oct 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bennypowers commented Oct 11, 2018

Uh oh!

Zhao-Andy Oct 12, 2018

Choose a reason for hiding this comment

Uh oh!

Zhao-Andy commented Oct 12, 2018

Uh oh!

bennypowers commented Oct 13, 2018

Uh oh!

bennypowers commented Nov 19, 2018

Uh oh!

Zhao-Andy commented Nov 19, 2018

Uh oh!

Uh oh!

glebec commented Sep 21, 2018 •

edited

Loading

Zhao-Andy commented Sep 27, 2018 •

edited

Loading

glebec Sep 30, 2018 •

edited

Loading

glebec Sep 30, 2018 •

edited

Loading

glebec commented Oct 11, 2018 •

edited

Loading

Zhao-Andy commented Oct 11, 2018 •

edited

Loading