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

GFM enhancement, do not remove _ (underscore) in words from html id tags #267

Closed
chadpowers opened this issue Jul 23, 2015 · 11 comments
Closed
Assignees

Comments

@chadpowers
Copy link

@chadpowers chadpowers commented Jul 23, 2015

I have another addition to the GFM parser. For headers involving involving underscores, i.e. for
### variable_name Github leaves the underscore in the html id attribute, i.e. Github produces id="variable_name" while kramdown createsid="variablename". Can you add this enhancement to the GFM parser to mimic Github's html id generation? Thanks!

@gettalong
Copy link
Owner

@gettalong gettalong commented Aug 8, 2016

@chadpowers I was looking into this, do you have a live example for this? I.e. source code and resulting HTML produced by Github?

@s3ththompson
Copy link

@s3ththompson s3ththompson commented Oct 25, 2016

You could perhaps just replace basic_generate_id with the implementation that GitHub uses, for example:

PUNCTUATION_REGEXP = /[^\p{Word}\- ]/u

def basic_generate_id(str)
  gen_id = str.downcase
  gen_id.gsub!(PUNCTUATION_REGEXP, '')
  gen_id.gsub!(' ', '-')
  gen_id
end

I've run into this issue as well. GitHub turns <h1>32-bit Integer</h1> into <h1 id="32-bit-integer">32-bit Integer</h1>, while Kramdown yields <h1 id="bit-integer">32-bit Integer</h1>.

@gettalong
Copy link
Owner

@gettalong gettalong commented Oct 25, 2016

@s3ththompson Yeah, the problem is that there is no spec for GFM, so we can only guess and use available live examples.

@gettalong
Copy link
Owner

@gettalong gettalong commented Nov 9, 2016

@parkr Is Jekyll and/or Github Pags using the kramdown parser or kramdown's GFM parser as default? If it is the latter we should probably be careful regarding changes in the ID generation to avoid problems with existing documents.

@parkr
Copy link
Contributor

@parkr parkr commented Nov 9, 2016

@gettalong Hi! It uses the GFM parser. Our defaults are kramdown.input: GFM with kramdown.hard_wrap: false. Thanks for checking! 😸

@gettalong
Copy link
Owner

@gettalong gettalong commented Nov 9, 2016

@parkr Thanks! What do you think: Should we make a hard change now, i.e. adopt Github's ID generation as pointed out by @s3ththompson in #267 (comment) or just change the two mentioned cases (numbers at start and underscores)?

Regarding the needed changes:

I have added some more test cases to a wiki page to see live examples - see https://github.com/gettalong/kramdown/wiki/GFM-test-case.

We will need to set the ID in the GFM parser itself since the normal automatic ID generation is done in the HTML converter.

@parkr
Copy link
Contributor

@parkr parkr commented Nov 10, 2016

@gettalong Anything to move the GFM parser closer to actual GFM is 👍 from me. /cc @benbalter @jldec if they have other thoughts.

@gettalong
Copy link
Owner

@gettalong gettalong commented Nov 13, 2016

@parkr Okay, then I will update the GFM parser so that the ID generation logic from #267 (comment) is used.

@gettalong
Copy link
Owner

@gettalong gettalong commented Nov 13, 2016

Since we have to do this in the parser (otherwise we would need to create a completely separate HTML converter just for GFM), the results will probably be not completely the same, ie. there may be some cases with different results.

@gettalong
Copy link
Owner

@gettalong gettalong commented Nov 13, 2016

@chadpowers @s3ththompson @parkr I have implemented this, see commit 257ec15 - can you test this out some more? The test cases from https://github.com/gettalong/kramdown/wiki/GFM-test-case work fine.

I intend to release the next version some time next week.

@gettalong
Copy link
Owner

@gettalong gettalong commented Nov 20, 2016

Will be in the release that goes out today.

@gettalong gettalong closed this Nov 20, 2016
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