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

make slugify function configurable #199

Merged
merged 5 commits into from
Jul 29, 2018
Merged

Conversation

cathayandy
Copy link
Contributor

See #198 #177

@codecov
Copy link

codecov bot commented Jul 27, 2018

Codecov Report

Merging #199 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
+ Coverage   99.71%   99.71%   +<.01%     
==========================================
  Files           1        1              
  Lines         351      352       +1     
  Branches       51       52       +1     
==========================================
+ Hits          350      351       +1     
  Misses          1        1
Impacted Files Coverage Δ
index.js 99.71% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc2f574...ff46ec3. Read the comment docs.

@quantizor
Copy link
Owner

What’s the reasoning for this? Based on the previous tests with Japanese characters, the browser supported them fine as an anchor.

@cathayandy
Copy link
Contributor Author

Because he closed his pull request, so I have to do it again.

At first I wanted to follow his solution(the uri-encoded solution), but after I read your conversations, I think the raw and the uri-encoded solution both have their own scenarios. Therefore, I don't want to introduce a specific solution to the source code.

Also, the user can pass the same slugify function to both a toc-generator and this renderer to make it consistent and controllable. By now I have to copy your slugify function into my source code and pass it to the toc-generator.

@quantizor
Copy link
Owner

quantizor commented Jul 27, 2018 via email

Copy link
Owner

@quantizor quantizor left a comment

Choose a reason for hiding this comment

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

Upon further consideration, I think this does make sense. I had one nitpick but otherwise looks good!

index.js Outdated
@@ -712,6 +712,8 @@ const PARSE_PRIORITY_MIN = 5;
export function compiler(markdown, options) {
options = options || {};
options.overrides = options.overrides || {};
options.slugify = typeof options.slugify === 'function' ?
Copy link
Owner

Choose a reason for hiding this comment

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

This can be simplified to options.slugify || slugify

@cathayandy
Copy link
Contributor Author

P.S. It may be a little bothering now since we have reached an agreement. But I think you may misunderstand my intention. Non-alphanumeric headings fails to have an id because of this:

https://github.com/probablyup/markdown-to-jsx/blob/bc2f57412332dc670f066320c0f38d0252e0f057/index.js#L272

It leads to an empty id if the heading is full of non-alphanumeric characters. Therefore, something has to be done.

Anyway, thank you very much and I really appreciate your help in resolving the problem!

@quantizor quantizor merged commit 30f9bb5 into quantizor:master Jul 29, 2018
@quantizor
Copy link
Owner

Thanks! Will probably cut a release in the next day or two.

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.

None yet

2 participants