Skip to content

Conversation

@SleepWalker
Copy link
Contributor

@SleepWalker SleepWalker commented Mar 11, 2018

Motivation

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

I've wrote some test, that should be enough on my opinion.

To check the fixes manually you need write the doc file with the following lines:

## тест тест тест
## 第一章

By rendering on master branch you will get following anchor links:

#
#

After this PR applied:

#тест-тест-тест
#第一章

Things to discuss

Old character mapping in toSlug()

I have found the following filtering rule in lib/core/toSlug.js:

  //  var accents = "àáäâèéëêìíïîòóöôùúüûñç";
  const accents =
    '\u00e0\u00e1\u00e4\u00e2\u00e8' +
    '\u00e9\u00eb\u00ea\u00ec\u00ed\u00ef' +
    '\u00ee\u00f2\u00f3\u00f6\u00f4\u00f9' +
    '\u00fa\u00fc\u00fb\u00f1\u00e7';

  const without = 'aaaaeeeeiiiioooouuuunc';

This code does not cover all possible characters, so I think it can be removed. But if we do so, it may be considered as breaking change, right? As for now I left it as is.

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 11, 2018
@SleepWalker SleepWalker force-pushed the heading-links-improvement branch 2 times, most recently from c2bff07 to 0a3757e Compare March 11, 2018 15:37
@SleepWalker SleepWalker force-pushed the heading-links-improvement branch from 0a3757e to 10c4207 Compare March 23, 2018 06:43
@JoelMarcey
Copy link
Contributor

@SleepWalker Hi. If we go with this, you will need to rebase since CHANGELOG.md has been changed since this PR was made. Maybe just don't worry about making changes to the change log and I can handle that in the next release?

@SleepWalker SleepWalker force-pushed the heading-links-improvement branch from 10c4207 to c2f2409 Compare April 16, 2018 04:45
@SleepWalker
Copy link
Contributor Author

@JoelMarcey pushed changes without touching CHANGELOG

@SleepWalker SleepWalker force-pushed the heading-links-improvement branch from c2f2409 to ef6c160 Compare April 16, 2018 05:02
@microbouji
Copy link
Contributor

The unicode handling looks good to me. Great work!

For the unique slugs part, not sure how it's meant to work, but currently this:

# baz
# baz 1
# baz

results in

#baz
#baz-1
#baz-1

I guess that's a negligible edge case.

More importantly though, we'll have to find a way to reuse this in the new on-page nav. anchors.js is a remarkable plugin, while getTOC.js (used by the on page nav) uses markdown-toc. I believe there are good reasons to keep them both like this, but rewriting one or the other to avoid doing the same thing twice could also make sense.

My suggestion is you separate this into two PRs: one for the unicode changes that can land immediately, and another to make slugs unique that will also take into consideration the on-page nav.

@SleepWalker SleepWalker force-pushed the heading-links-improvement branch from ef6c160 to dbfbe05 Compare April 17, 2018 03:29
@SleepWalker
Copy link
Contributor Author

For the unique slugs part, not sure how it's meant to work, but currently this:

Oh, thank you. I have missed that case. The correct ids should be the following:

#baz
#baz-1
#baz-2

I have removed commit with headings uniqueness logic from current branch and will submit it as a separate PR including support for on-page nav.

Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

This is great @SleepWalker. Thanks!

Thank you @microbouji for your review.

@JoelMarcey JoelMarcey merged commit 1642c07 into facebook:master Apr 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Signed Facebook CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

onPageNav links not working for headings written in Chinese

4 participants