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

Fix Heading ID for Unicode text #1374

Closed
wants to merge 2 commits into from
Closed

Fix Heading ID for Unicode text #1374

wants to merge 2 commits into from

Conversation

k-utsumi
Copy link

@k-utsumi k-utsumi commented Nov 7, 2018

I think defining nonWordCharacters will solve it.

Reference: atom nonWordCharacters

@UziTech
Copy link
Member

UziTech commented Nov 7, 2018

according to the spec:

When specified on HTML elements, the id attribute value must be unique amongst all the IDs in the element's tree and must contain at least one character. The value must not contain any ASCII whitespace.

so couldn't we change it to /\s+/g?

@UziTech
Copy link
Member

UziTech commented Nov 7, 2018

I suppose we would want to remove " as well so /["\s]+/g

All other characters seem to work fine in ids on chrome.

@k-utsumi
Copy link
Author

k-utsumi commented Nov 8, 2018

Maybe, need to consider the behavior of shared URL with fragment (section ID).

e.g. ## how to resolve #123 ? or ...


I'm sorry, I closed it by mistake in the mark down test.

@k-utsumi k-utsumi closed this Nov 8, 2018
@k-utsumi k-utsumi reopened this Nov 8, 2018
@styfle
Copy link
Member

styfle commented Nov 12, 2018

This feature (header id) is being removed in PR #1354 so I don't think it's worth pursuing in this PR.

@k-utsumi
Copy link
Author

Thanks😄

@k-utsumi k-utsumi closed this Nov 13, 2018
@k-utsumi k-utsumi reopened this Dec 6, 2018
@k-utsumi
Copy link
Author

k-utsumi commented Dec 6, 2018

I want to eliminate this problem quickly, so I reopened with a simple code adjustment.
If merged, I’ll make same PR for other services I use.

@UziTech
Copy link
Member

UziTech commented Dec 6, 2018

@styfle do we want to add a seenIds array to prevent collisions on this pr?

@styfle
Copy link
Member

styfle commented Dec 6, 2018

@UziTech If we are not going to remove it, we could go all-out and implement like so:

function Slugger () {	
  this.seen = {};	
}	

Slugger.prototype.slug = function (value) {	
  var slug = value	
    .toLowerCase()	
    .trim()	
    .replace(/[\u2000-\u206F\u2E00-\u2E7F\\'!"#$%&()*+,./:;<=>?@[\]^`{|}~]/g, '')	
    .replace(/\s/g, '-');	
  var count = this.seen.hasOwnProperty(slug) ? this.seen[slug] + 1 : 0;	
  this.seen[slug] = count;	
   if (count > 0) {	
    slug = slug + '-' + count;	
  }	
   return slug;	
};

I think we'll need some unit tests to see what will and will not match as far as unicode goes.

That was one of the reasons why I was trying to tear it out, so the implementation and tests could live in github-slugger package.

@UziTech
Copy link
Member

UziTech commented Dec 6, 2018

I think not having dependencies is one of the big benefits of marked so if we want to have ids be part of gfm we will need to incorporate slugger into our code.

@styfle
Copy link
Member

styfle commented Dec 9, 2018

@k-utsumi Would you like to update this PR with the slugger or should I create a new PR?

@k-utsumi
Copy link
Author

@styfle
Since I can't understand the change correctly,
Could you edit the code directory or create a new PR?

@styfle
Copy link
Member

styfle commented Dec 20, 2018

Yes, I will create a PR this weekend with the appropriate change.

@styfle styfle mentioned this pull request Dec 20, 2018
4 tasks
@styfle
Copy link
Member

styfle commented Dec 20, 2018

Closing in favor of #1401

@styfle styfle closed this Dec 20, 2018
@k-utsumi k-utsumi deleted the fix-heading-id branch January 7, 2019 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants