Skip to content

Conversation

@Xenonym
Copy link
Contributor

@Xenonym Xenonym commented Dec 13, 2018

What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [X] Bug fix

Part of #413.

What is the rationale for this request?
[email protected] creates anchor tags with URL encoded characters when the heading contains special characters. For example, "Guideline: Avoid Unsafe Shortcuts" becomes guideline%3A-avoid-unsafe-shortcuts. This breaks the use of jQuery to scroll to the correct anchor as jQuery will not be able to locate elements with special characters in the id. This can be fixed by using document.getElementById to locate the element instead.

What changes did you make? (Give an overview)
Upgrade markdown-it-anchor to ^5.0.0 and use document.getElementById to locate the element to scroll to instead of jQuery.

Provide some example code that this change will affect:

## Guideline: Avoid Unsafe Shortcuts

Testing instructions:

  1. Create a markdown document with headers that have special characters eg. :. The anchor link should be able to scroll normally despite being generated from [email protected].

@Xenonym Xenonym changed the title Upgrade markdown-it-anchor to 5.0.0 and fix anchor tag scrolling [WIP] Upgrade markdown-it-anchor to 5.0.0 and fix anchor tag scrolling Dec 13, 2018
@Xenonym Xenonym changed the title [WIP] Upgrade markdown-it-anchor to 5.0.0 and fix anchor tag scrolling Upgrade markdown-it-anchor to 5.0.0 and fix anchor tag scrolling Dec 13, 2018
@yamgent yamgent self-requested a review December 14, 2018 03:56
@Xenonym Xenonym changed the title Upgrade markdown-it-anchor to 5.0.0 and fix anchor tag scrolling [WIP] Upgrade markdown-it-anchor to 5.0.0 and fix anchor tag scrolling Dec 15, 2018
@Xenonym Xenonym changed the title [WIP] Upgrade markdown-it-anchor to 5.0.0 and fix anchor tag scrolling Upgrade markdown-it-anchor to 5.0.0 and fix anchor tag scrolling Dec 16, 2018
@Xenonym Xenonym closed this Dec 16, 2018
@Xenonym Xenonym reopened this Dec 16, 2018
Copy link
Member

@yamgent yamgent left a comment

Choose a reason for hiding this comment

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

Thanks, looks good now.

Please squash the PR to two commits:

  • Upgrade markdown-it-anchor to 5.0.0: Should contain the package.json, package-lock.json, and the change in the anchors for the html files.
  • Fix URL anchor scrolling: Should contain the changes in setup.js.

On a side note, I edited your initial post, because using the word "Resolve" will close the issue #413, but in this case the issue needs to be fixed on two different projects (this PR fixes one of them), so the issue shouldn't be closed yet. But you don't have to worry about the other project for now (it is a trivial fix), so this PR is basically complete.

@Xenonym
Copy link
Contributor Author

Xenonym commented Dec 17, 2018

Thanks for the review and comments @yamgent! I have squashed the commits as requested. Noted on the other project as well.

@yamgent yamgent added this to the v1.14.3 milestone Dec 18, 2018
@yamgent yamgent merged commit 4fbe485 into MarkBind:master Dec 18, 2018
@Xenonym Xenonym deleted the markdownit-anchor-5.0.0-urlencode-fix branch December 18, 2018 05:33
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.

2 participants