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

Provide anchors with headings if Github flowered markdown #131

Closed
wants to merge 3 commits into from

Conversation

Swaagie
Copy link

@Swaagie Swaagie commented Mar 5, 2013

fixes #89

I'd really like to fix some of the tests that will be broken now. However, I fail to see an easy way to turn on/off several options, like gfm. It would be best add some tests without gfm and patch up the 5 that will break as result of this change.

@lepture
Copy link
Contributor

lepture commented Mar 6, 2013

@Swaagie This could be easily customized with renderer feature, which I provided at #129 .

@Swaagie
Copy link
Author

Swaagie commented Mar 7, 2013

I agree that would be a nice addition, currently just went with the default rendering of github. You can do a lot with some CSS and an empty anchor. But I guess some people would rather have id's on headings or perhaps an alternative inline element. I'll try to bug @chjj on IRC to review this stuff.

var link = this.token.text
.toLowerCase()
.replace(/\s/g, '-')
.replace(/[^a-z0-9-_\:\.]/g, '');
Copy link

Choose a reason for hiding this comment

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

Note that including .s and :s will break CSS selecting.

Copy link
Author

Choose a reason for hiding this comment

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

how does this break selecting? link is only used as href and name

Copy link

Choose a reason for hiding this comment

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

Some (bootstrap's scrollspy for example) might want to select elements like this: a[href=the-converted-id]

viz. http://stackoverflow.com/a/72577, also a bit surprisingly, name is supposed to be deprecated: http://www.w3schools.com/tags/att_a_name.asp

Copy link
Author

Choose a reason for hiding this comment

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

I can see this being the case for general use cases, however this
addition is specifically tailored to Github Flavored Markdown, and as
such following their conventions. Also note that it's using the heading
text not the id. I can see the scrollspy working like this, but if I'm
missing something tell me please

On 03/27/2013 06:40 PM, Michal Srb wrote:

In lib/marked.js:

@@ -820,9 +820,22 @@ Parser.prototype.tok = function() {
return '


\n';
}
case 'heading': {

  •  var internal = '';
    
  •  if (this.options.gfm) {
    
  •    var link = this.token.text
    
  •      .toLowerCase()
    
  •      .replace(/\s/g, '-')
    
  •      .replace(/[^a-z0-9-_:.]/g, '');
    

Some (bootstrap's scrollspy for example) might want to select elements
like this: a[href=the-converted-id]


Reply to this email directly or view it on GitHub
https://github.com/chjj/marked/pull/131/files#r3555907.

@micky2be
Copy link

micky2be commented Jul 2, 2013

looking forward to see that PR merged

@chjj
Copy link
Member

chjj commented Aug 2, 2013

Closed in favor of #181.

@chjj chjj closed this Aug 2, 2013
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.

Add support for element ID generation
5 participants