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

Headers don't render properly if there is a link in them #16

Closed
kbaley opened this issue Dec 8, 2015 · 7 comments
Closed

Headers don't render properly if there is a link in them #16

kbaley opened this issue Dec 8, 2015 · 7 comments
Labels
bug

Comments

@kbaley
Copy link

@kbaley kbaley commented Dec 8, 2015

For example:

## Quick Start [with a link](http://github.com)

Renders like so:

image

This appears to be due to the automatic inserting of a bookmark into each header, which I'm not sure is necessary.

@jakeecolution
Copy link

@jakeecolution jakeecolution commented Dec 8, 2015

This also shows up on the hexojs/site and I have seen the problem on my personal page.

@Xuanwo Xuanwo added the bug label Dec 9, 2015
@Xuanwo
Copy link
Contributor

@Xuanwo Xuanwo commented Dec 9, 2015

Yes, it's a bug, let's fix it.

@cgmartin
Copy link
Contributor

@cgmartin cgmartin commented Dec 9, 2015

Some additional details from debugging this...

Given a test-post.md:

title: Test Post
tags:
---
## [hexo-server]

[hexo-server]: https://github.com/hexojs/hexo-server

Line 20 of the renderer, text argument, comes in as:

<a href="https://github.com/hexojs/hexo-server">hexo-server</a>

...with the returned header link result of:

<h2 id="hexo-server"><a href="#hexo-server" class="headerlink" title="<a href="https://github.com/hexojs/hexo-server">hexo-server</a>"></a><a href="https://github.com/hexojs/hexo-server">hexo-server</a></h2>

As a proposed solution, what about skipping the header link if an anchor already exists in the string? Something like this as a replacement for line 31:

  var link
  if (text.substring(0, 2) === '<a') {
    // text already contains a link
    link = '<h' + level + ' id="' + id + '">' + text + '</h' + level + '>';
  } else {
    // add headerlink
    link = '<h' + level + ' id="' + id + '"><a href="#' + id + '" class="headerlink" title="' + text + '"></a>' + text + '</h' + level + '>';
  }
  return link;

I'm new to hexo, so I'm unsure what other impacts this may have on other parts of the system (if any).

@kbaley
Copy link
Author

@kbaley kbaley commented Dec 9, 2015

Also new to hexo here but my suggestion would be to remove the automatic bookmark completely. Or at the very least, make it an option that can be turned on if necessary. I don't see a lot of use for it in the wild so including it "just in case" adds little value.

That's the approach taken by this renderer which I've switched to for now.

@cgmartin
Copy link
Contributor

@cgmartin cgmartin commented Dec 9, 2015

Bah, ignore my proposed solution above. I just realized that the header link is not wrapping the text itself. The problem is in the title="..." attribute. We could potentially extract the title from a text string with an anchor:

https://github.com/hexojs/hexo-renderer-marked/blob/master/lib/renderer.js#L31

// Strip HTML from the text in title attribute:
return '<h' + level + ' id="' + id + '"><a href="#' + id + '" class="headerlink" title="' + stripHTML(text) + '"></a>' + text + '</h' + level + '>';
@cgmartin
Copy link
Contributor

@cgmartin cgmartin commented Dec 9, 2015

@kbaley my preference would be to keep the feature (an option to enable/disable would be OK for me). I find being able to share links directly to header sections to be generally helpful. It is annoying when I cannot find an anchor tag on a very long page when I want to share a direct link to a section. It is also being used in conjunction with the table of contents on the hexo docs pages. Just my 2 cents.

@Xuanwo
Copy link
Contributor

@Xuanwo Xuanwo commented Dec 20, 2015

Closed for merge the patch.

@Xuanwo Xuanwo closed this Dec 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.