Skip to content

Swap showdown-ghost fork for markdown-it.#6352

Closed
nybblr wants to merge 17 commits intoTryGhost:masterfrom
nybblr:switch-to-markdown-it
Closed

Swap showdown-ghost fork for markdown-it.#6352
nybblr wants to merge 17 commits intoTryGhost:masterfrom
nybblr:switch-to-markdown-it

Conversation

@nybblr
Copy link
Copy Markdown

@nybblr nybblr commented Jan 16, 2016

Swapping out showdown-ghost fork for markdown-it.

Quite a few plugins folks could plug and play beyond core: https://www.npmjs.com/browse/keyword/markdown-it-plugin

#6125

TODO:

  • Add footnotes plugin
  • Check feature parity
  • Check test suite
  • Fix header IDs spec
  • Use Browserify NPM instead of Bower markdown-it
  • Ghost image preview plugin + tests
  • DRY markdown rendering options

@nybblr
Copy link
Copy Markdown
Author

nybblr commented Jan 16, 2016

Investigating https://github.com/valeriangalliat/markdown-it-anchor for header anchors:

It has:

  • a good test suite
  • recent activity
  • most issues closed
  • extensive options

@nybblr nybblr force-pushed the switch-to-markdown-it branch from 8c092f8 to 8e954be Compare January 16, 2016 18:54
* <s> and <del> are considered equivalent, and carry the same styling by
* default. Casper applies no styling.
* Showdown seems to be in the wrong here: to opt-in to HTML, most specs
* require that a new line start with "<". Starting a line with regular
* text, then inserting HTML in this case is undefined behavior.
@nybblr
Copy link
Copy Markdown
Author

nybblr commented Jan 16, 2016

4 specs failing, all related to the custom image placeholder functionality.

All the other tests failed due to:

  • Differences in whitespace on Showdown's part (technically incorrect, like unnecessary spaces before line breaks or 2 extra spaces after a header). In each case, I validated that markdown-it follows correct behavior on newlines + spaces.
  • Difference in tag (just <s> vs <del>, which are equivalent)
  • Instances of invalid markdown input that showdown was too lenient on (esp. #Header; most implementations and specs call for a space)

* Disable ![] image syntax, using ![]() instead.
* Disable image footnotes (seems like a random edge case turned feature)
@nybblr
Copy link
Copy Markdown
Author

nybblr commented Jan 17, 2016

Dropped two edge case behaviors:

  • Image footnote (behavior seems ill-defined)
  • ![] syntax to trigger image uploader (throws parse engines for a loop), instead use the full ![]() to get the nice uploader.

@nybblr
Copy link
Copy Markdown
Author

nybblr commented Jan 17, 2016

Test suite is passing locally, any thoughts on why Travis isn't running, @ErisDS or @acburdine?

@acburdine
Copy link
Copy Markdown
Member

Not sure exactly why, I'd try fixing the merge conflicts and force-pushing and seeing if that triggers a build. Sometimes Travis has glitches 😄

@acburdine
Copy link
Copy Markdown
Member

Although @ErisDS can comment more on this, I'm not sure whether Ghost is looking for a replacement to its markdown converter at the moment. There's a lot of work going on to replace the editor with something more configurable/customizable. You can see more details here.

@ErisDS
Copy link
Copy Markdown
Member

ErisDS commented Jan 17, 2016

Hi @nybblr, I appreciate you've spent some time on this, but it's really worth checking in with a big project like ours before trying to replace a major dependency. Our editor is, after all, our centre piece!

We're currently in the process of replacing the editor.

@ErisDS ErisDS closed this Jan 17, 2016
@nybblr
Copy link
Copy Markdown
Author

nybblr commented Jan 18, 2016

@ErisDS that looks neat, thanks for the link. I'm excited to see where the editor heads!

Still, that seems a long way off (and in the interest of experimentation, could be awesome/not so awesome). This PR is a drop-in replacement that maintains feature parity, gets us off an outdated fork, and gives developers options to customize their markdown plugins.

This would also immediately close most of these editor issues due to invalid markdown rendering: https://github.com/TryGhost/Ghost/labels/editor

I think it would be a win-win to do something like this while we wait for that awesome new editor to be vetted, tested, and accepted into stable. And since some folks may inevitably not be crazy about the move from markdown, this would give them something to roll back too without being stuck on a buggy showdown fork.

Thoughts?

@nuclearpengy
Copy link
Copy Markdown

❤️ Markdown

@ErisDS
Copy link
Copy Markdown
Member

ErisDS commented Jan 19, 2016

Regardless of which route we take, the editor will be whole-sale replaced within the next couple of months. It's our main ongoing project.

I appreciate the desire to get off of the current showdown fork. There is an open issue for it: #5598 and it's something I was actively working on until recently. I can remove the 'in-progress' labels and someone else can look at doing this, but right now I don't see that there's justification for switching markdown libraries when the editor is about to change.

To address some of the other comments/questions/concerns:

This PR is a drop-in replacement that maintains feature parity

From viewing a couple of the changes to tests, it seems to me that this statement is factually incorrect.

gives developers options to customize their markdown plugins

Not sure how this is the case?

And since some folks may inevitably not be crazy about the move from markdown, this would give them something to roll back too without being stuck on a buggy showdown fork.

Just no. No, no no. And no. Trying to sell making this change as a way to encourage people not to upgrade Ghost in the future is so backwards its untrue. We're building a new, more powerful and more customisable editor to solve the problems with the existing one.

@kevinansfield
Copy link
Copy Markdown
Member

And since some folks may inevitably not be crazy about the move from markdown

There will be no need for users who are wed to markdown to move away from it, quite the opposite, the new editor will eventually allow drop-in replacement of markdown parsers/markdown editing interfaces. The move is towards a better default UX for non-technical users whilst also increasing flexibility and options for developers/power-users.

@nuclearpengy
Copy link
Copy Markdown

@kevinansfield sounds rad. I can't imagine it's been easy. :)

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.

5 participants