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.

Sign up for GitHub

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

Jump to bottom

bugfix: Fix hrefs for headings with angular brackets #1089

Merged
merged 3 commits into from
Apr 1, 2020
Merged

bugfix: Fix hrefs for headings with angular brackets #1089

merged 3 commits into from
Apr 1, 2020

Conversation

yash-chowdhary
Copy link
Contributor

@yash-chowdhary yash-chowdhary commented Mar 4, 2020 •
edited
Loading

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

• [x] Bug fix

Fixes #1076

What is the rationale for this request?
Presence of angular brackets in headings creates undesirable href links.

What changes did you make? (Give an overview)
Removed occurrences of the strings 'and lt' and 'and gt' that are passed to slugify.

Provide some example code that this change will affect:

customReplacements: [
['and lt', ''],
['and gt', ''],
],
});

New fix:

let cleanedContent = textContent.replace(/</g, '');
cleanedContent = cleanedContent.replace(/>/g, '');

Is there anything you'd like reviewers to focus on?
n/a

Proposed commit message: (wrap lines at 72 characters)

The hrefs of headings with angular brackets are incorrect because
markdown-it escapes the characters to "<" and ">" and the
subsequent slugify operation converts it to "and-lt" or "and-gt".

Let's fix this by removing the occurrences of the strings 
"<" and ">" before the slugify operation

@yash-chowdhary
Copy link
Contributor Author

@ang-zeyu could I get your review on this?

@ang-zeyu
Copy link
Contributor

Realised that the con of this fix is that you cant type and lt 😧

headerId = headerId.replace(/[-]?and-lt/g, '');
Perhaps your original fix, but placed before the slugify call and adapted to only handle &lt/gt; would be better?

@yash-chowdhary
Copy link
Contributor Author

Realised that the con of this fix is that you cant type and lt

Good catch 😅

Perhaps your original fix, but placed before the slugify call and adapted to only handle &lt/gt; would be better?

Yup, I've now changed it to use the previously suggested fix. This will remove all occurrences of < and > from the headings after markdown-it's parsing.

ang-zeyu
ang-zeyu reviewed Mar 16, 2020
View reviewed changes
src/lib/markbind/src/parser.js Outdated
@@ -263,7 +263,9 @@ class Parser {

if ((/^h[1-6]$/).test(node.name) && !node.attribs.id) {
const textContent = utils.getTextContent(node);
const slugifiedHeading = slugify(textContent, { decamelize: false });
let cleanedContent = textContent.replace(/</g, '');
cleanedContent = cleanedContent.replace(/>/g, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, I've now changed it to use the previously suggested fix.

👍

Why not combine it into one though <|> :o
A comment here explaining why we clean <> in particular might be nice too

@yash-chowdhary yash-chowdhary added the pr.BugFix 🐛 Fixes correct a programming error/assumption label Apr 1, 2020
@yash-chowdhary
Copy link
Contributor Author

@yash-chowdhary i think you may see if this PR is ready to be merged according to https://markbind.org/devdocs/devGuide/maintainerGuide.html

I suppose that this PR is trivial enough to be merged after 1 approval

@yash-chowdhary yash-chowdhary added this to the v2.13.2 milestone Apr 1, 2020
@nbriannl
Copy link
Contributor

nbriannl commented Apr 1, 2020 •
edited
Loading

sorry for deleting comment.

@yash-chowdhary i think you may see if this PR is ready to be merged according to https://markbind.org/devdocs/devGuide/maintainerGuide.html

I suppose that this PR is trivial enough to be merged after 1 approval

yeah, i don't really know if anything will regress after this. No clean unit test for this because these functionality lies within _parse function. 🤢

If anything, the fact that this link
https://deploy-preview-1089--markbind-master.netlify.com/userGuide/reusingContents.html#specifying-variables-in-an-include
now directs the page correctly means that your implementation is correct. 😄

@yash-chowdhary
Copy link
Contributor Author

If anything, the fact that this link
https://deploy-preview-1089--markbind-master.netlify.com/userGuide/reusingContents.html#specifying-variables-in-an-include
now directs the page correctly means that your implementation is correct.

Yup, it can be merged. 🙂

@yash-chowdhary yash-chowdhary merged commit 69ad5b1 into MarkBind:master Apr 1, 2020
Tejas2805 added a commit to Tejas2805/markbind that referenced this pull request Apr 9, 2020
@Tejas2805
Merge branch 'master' of https://github.com/MarkBind/markbind
9792d68 
* 'master' of https://github.com/MarkBind/markbind: (41 commits)
  Document adding new site content in DG (MarkBind#1153)
  Add relative date feature (MarkBind#908)
  Use <br> to separate lines of code block (MarkBind#1176)
  Parse popovers for footnotes (MarkBind#1155)
  Resolve comments
  Expand test extensions and fix whitespace checks (MarkBind#1156)
  Address comments
  Upgrade js-beautify and provide option to turn it off (MarkBind#1116)
  Normalize inline puml line ending before hashing (MarkBind#1174)
  Update tests (MarkBind#1178)
  Remove fixed bugs from test\functional\test_site\bugs\index.md` (MarkBind#1148)
  Fix bug in Search for UG and DG (MarkBind#1147)
  Add inline puml support (MarkBind#1100)
  Fix hrefs for headings with angular brackets (MarkBind#1089)
  Update tests for 2.13.1 (MarkBind#1169)
  2.13.1
  Update vue-strap version to v2.0.1-markbind.39
  Fix fontawsome icons don't show underlines to indicate modal/tooltip (MarkBind#1133)
  2.13.0
  Update test files
  ...
marvinchin pushed a commit that referenced this pull request Apr 10, 2020
@yash-chowdhary
Fix hrefs for headings with angular brackets (#1089)
481b811 
```
The hrefs of headings with angular brackets are incorrect because
markdown-it escapes the characters to "&lt;" and "&gt;" and the
subsequent slugify operation converts it to "and-lt" or "and-gt".

Let's fix this by removing the occurrences of the strings 
"&lt;" and "&gt;" before the slugify operation
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ang-zeyu ang-zeyu ang-zeyu approved these changes

@marvinchin marvinchin Awaiting requested review from marvinchin

Assignees
No one assigned
Labels
pr.BugFix 🐛 Fixes correct a programming error/assumption
Projects
None yet
Milestone
v2.14.0
Development

Successfully merging this pull request may close these issues.

Improper linking of headings that have part of text enclosed within "<>"
3 participants
@yash-chowdhary @ang-zeyu @nbriannl