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

# Paste **MarkDown** #2531

Merged
merged 2 commits into from Sep 28, 2017

Conversation

Projects
None yet
4 participants
@iseulde
Member

iseulde commented Aug 24, 2017

This adds MarkDown parsing to the paste handling if the posted content is plain text. It won't parse anything if you don't paste from a plain text editor.

This is a WIP... Still need to have a look how to handle inline text better and at the parsers available. Any suggestions? Showdown is is used by Ghost so I picked that for now.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Aug 25, 2017

Member

What's the impact on bundle size by pulling in showdown ? Also curious if this is something best left for plugin territory...

Member

aduth commented Aug 25, 2017

What's the impact on bundle size by pulling in showdown ? Also curious if this is something best left for plugin territory...

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Aug 25, 2017

Member

10.7 kb gzipped... I'm fine either way, I think this is a nice surprise enhancement. I mainly want to show here that it's easily possible to add, and that not everything would run through the parser, only plain text.

Also this may be a place then where we would want to add a filter for plugins.

Member

iseulde commented Aug 25, 2017

10.7 kb gzipped... I'm fine either way, I think this is a nice surprise enhancement. I mainly want to show here that it's easily possible to add, and that not everything would run through the parser, only plain text.

Also this may be a place then where we would want to add a filter for plugins.

@dmsnell

This comment has been minimized.

Show comment
Hide comment
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 22, 2017

Codecov Report

Merging #2531 into master will decrease coverage by 0.03%.
The diff coverage is 22.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2531      +/-   ##
==========================================
- Coverage   33.69%   33.65%   -0.04%     
==========================================
  Files         185      185              
  Lines        5594     5615      +21     
  Branches      976      980       +4     
==========================================
+ Hits         1885     1890       +5     
- Misses       3141     3155      +14     
- Partials      568      570       +2
Impacted Files Coverage Δ
blocks/library/code/index.js 57.14% <ø> (ø) ⬆️
blocks/library/preformatted/index.js 50% <ø> (ø) ⬆️
blocks/api/paste/utils.js 82.97% <0%> (-14.53%) ⬇️
blocks/api/paste/test/integration/index.js 100% <100%> (ø) ⬆️
blocks/editable/index.js 10.5% <20%> (+0.18%) ⬆️
blocks/api/paste/index.js 79.31% <40%> (-8.69%) ⬇️
components/form-file-upload/index.js 66.66% <0%> (-4.77%) ⬇️

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 a93c553...aa73265. Read the comment docs.

codecov bot commented Sep 22, 2017

Codecov Report

Merging #2531 into master will decrease coverage by 0.03%.
The diff coverage is 22.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2531      +/-   ##
==========================================
- Coverage   33.69%   33.65%   -0.04%     
==========================================
  Files         185      185              
  Lines        5594     5615      +21     
  Branches      976      980       +4     
==========================================
+ Hits         1885     1890       +5     
- Misses       3141     3155      +14     
- Partials      568      570       +2
Impacted Files Coverage Δ
blocks/library/code/index.js 57.14% <ø> (ø) ⬆️
blocks/library/preformatted/index.js 50% <ø> (ø) ⬆️
blocks/api/paste/utils.js 82.97% <0%> (-14.53%) ⬇️
blocks/api/paste/test/integration/index.js 100% <100%> (ø) ⬆️
blocks/editable/index.js 10.5% <20%> (+0.18%) ⬆️
blocks/api/paste/index.js 79.31% <40%> (-8.69%) ⬇️
components/form-file-upload/index.js 66.66% <0%> (-4.77%) ⬇️

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 a93c553...aa73265. Read the comment docs.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Sep 22, 2017

Member

Okay, so this will now do two things.

  1. We're going to look at the plain text the clipboard API is giving us if the HTML is plain. If the source (editor) is plain text, this will be more reliable than any HTML made up by the browser.
  2. If we have some reliable plain text, then we convert any markdown in it. If the user pastes HTML, this will never run.

The PR uses showdown, but we can easily use anything else later of course.

Member

iseulde commented Sep 22, 2017

Okay, so this will now do two things.

  1. We're going to look at the plain text the clipboard API is giving us if the HTML is plain. If the source (editor) is plain text, this will be more reliable than any HTML made up by the browser.
  2. If we have some reliable plain text, then we convert any markdown in it. If the user pastes HTML, this will never run.

The PR uses showdown, but we can easily use anything else later of course.

@iseulde iseulde requested a review from aduth Sep 22, 2017

@iseulde iseulde merged commit 410caac into master Sep 28, 2017

3 checks passed

codecov/project 33.65% (-0.04%) compared to a93c553
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Sep 28, 2017

Member

Merged after 👍 form @mtias and @dmsnell.

Member

iseulde commented Sep 28, 2017

Merged after 👍 form @mtias and @dmsnell.

@iseulde iseulde deleted the try/paste-markdown branch Sep 28, 2017

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 28, 2017

Member

We should have updated package-lock.json here from the added dependency. npm install in a clean clone while running npm 5+ will introduce changes.

Member

aduth commented Sep 28, 2017

We should have updated package-lock.json here from the added dependency. npm install in a clean clone while running npm 5+ will introduce changes.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 28, 2017

Member

Updated in ff1ef2b

Member

aduth commented Sep 28, 2017

Updated in ff1ef2b

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Sep 28, 2017

Member

Thanks @aduth! Sorry about that, wasn't aware of package-lock.

Member

iseulde commented Sep 28, 2017

Thanks @aduth! Sorry about that, wasn't aware of package-lock.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 28, 2017

Member

No worries, since we don't explicitly target the version of Node that comes bundled with npm5+ (until LTS release in October), we maybe shouldn't have the package-lock.json at all, but the niceties of 5+ are enough for me to want to run it anyways 😄

Member

aduth commented Sep 28, 2017

No worries, since we don't explicitly target the version of Node that comes bundled with npm5+ (until LTS release in October), we maybe shouldn't have the package-lock.json at all, but the niceties of 5+ are enough for me to want to run it anyways 😄

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 29, 2017

Contributor

@aduth The package-lock.json locks only the dependencies of the dependencies since 5.1.0 which makes it useless IMO. (it's not the same as shrinkwrap or yarn.lock)

Contributor

youknowriad commented Sep 29, 2017

@aduth The package-lock.json locks only the dependencies of the dependencies since 5.1.0 which makes it useless IMO. (it's not the same as shrinkwrap or yarn.lock)

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 29, 2017

Member

package-lock.json locks only the dependencies of the dependencies since 5.1.0 which makes it useless IMO

That still seems like a pretty good benefit on its own, if you ask me.

Part of it is that it's automatic, so it becomes a nuisance to be merely running npm install and having changes introduced. If we really didn't care, I'd want to find a way to turn that off.

But then, I saw what you'd referred to come past, but I'd have to think it still provides some benefit; surely they wouldn't make the thing useless? 😄 I'll have to do some reading over the weekend.

Member

aduth commented Sep 29, 2017

package-lock.json locks only the dependencies of the dependencies since 5.1.0 which makes it useless IMO

That still seems like a pretty good benefit on its own, if you ask me.

Part of it is that it's automatic, so it becomes a nuisance to be merely running npm install and having changes introduced. If we really didn't care, I'd want to find a way to turn that off.

But then, I saw what you'd referred to come past, but I'd have to think it still provides some benefit; surely they wouldn't make the thing useless? 😄 I'll have to do some reading over the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment