-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Replace "marked" with "markdown-it" #13623
Conversation
@nreese, can you give some context into the reason for this change? |
@tylersmalley |
Thanks @nreese, somehow I read over the linked issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems to work as intended. Code looks straightforward. LGTM! Nice tests.
@@ -149,7 +149,7 @@ | |||
"less": "2.7.1", | |||
"less-loader": "2.2.3", | |||
"lodash": "3.10.1", | |||
"marked": "0.3.6", | |||
"markdown-it": "8.3.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few differences that I'm seeing between marked
/markdown-it
, using the following markdown:
Ordered
1. Lorem ipsum dolor sit amet
2. Consectetur adipiscing elit
3. Integer molestie lorem at massa
1. You can use sequential numbers...
1. ...or keep all the numbers as `1.`
Start numbering with offset:
57. foo
1. bar
Autoconverted link https://github.com/nodeca/pica (enable linkify to see)
I'm seeing the following with marked
:
And the following with markdown-it
:
I believe that markdown-it
is doing the numbering "correctly" or it at least matches what GitHub does; however, I figured this should at least be noted somewhere.
Regarding the auto-linking, this is something that marked
used to automatically include, do we want to support this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its an easy configuration change to enable. I am +1 on enabling. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm +1 on enabling auto linking, for what it's worth. If someone is putting a raw link into a markdown document, it's hard to imagine the circumstance where they want people to copy and paste that URL rather than just click it.
If we're happier with the markdown-it number approach, then we should pull the bandaid off. This is is a major version change anyway, so breaking changes can happen.
Speaking of breaking changes, @nreese can you add the switch in markdown processors to the breaking change docs in this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* add markdown functional test * update markdown vis to use markdown-it * migrate markdown angular filter to markdown-it * place other uses of marked and remove dependency * update breaking changes documenation and set linkify to true
* add markdown functional test * update markdown vis to use markdown-it * migrate markdown angular filter to markdown-it * place other uses of marked and remove dependency * update breaking changes documenation and set linkify to true
Fixes #13425
Replaces
marked
dependency withmarkdown-it
backported to 6.0 #13666
backported to 6.1 #13667