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.

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

Support pasting markdown #17

Merged
merged 9 commits into from
Apr 16, 2017
Merged

Support pasting markdown #17

merged 9 commits into from
Apr 16, 2017

Conversation

nil1511
Copy link

@nil1511 nil1511 commented Feb 27, 2017

Added Support for pasting markdown.

How does it work?

We get the pasted text using draft-js api handlePastedText. Suppose the pasted text is

Hello World
This is _italic_
This end with *space*
* Strike
End

Every letter is iterated and push into buffer array. On encounter of oneOf(' ', '*', '_'). It push the text from buffer into editorState then work as handleBeforeInput is called then use the newEditorState and clear buffer. similarly in case of enter(return, newline) work as handleReturn is called. In case of end buffer is push into the editorState and finally the new editorState is set.

src/index.js Outdated
if (INLINE_STYLE_CHARACTERS.indexOf(text[i]) >= 0) {
newEditorState = addText(newEditorState, buffer.join('') + text[i]);
newEditorState = _handleCharacter(newEditorState, text[i]);
buffer.length = 0;
Copy link
Owner

Choose a reason for hiding this comment

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

What does this line intended to?

Copy link
Author

Choose a reason for hiding this comment

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

While typing Inline transformation were taking place we space is entered.
But in case on pasting text.
"_hello_ " was transformed to hello.
But "_hello_" was not being transformed. Thus while pasting INLINE_STYLE_CHARACTERS also checked.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, that makes sense.

I meant I'm thinking assigning length property to Array instance will affect nothing.

Can I ask what intended to do in this line 148?

Copy link
Author

Choose a reason for hiding this comment

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

buffer.length = 0 will reset the array to empty without changing it's refrence thus it would work as const. Otherwise we can do let buffer = [] and buffer = [] instead of buffer.length = 0.

Copy link
Author

Choose a reason for hiding this comment

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

Updated to let version I thought buffer.length was faster. I was wrong.
https://jsperf.com/array-destroy

@ngs
Copy link
Owner

ngs commented Feb 27, 2017

  • Can you please describe more about your changes?
  • Your code is not test covered, can you please write test code to cover all changes?

src/index.js Outdated
@@ -1,3 +1,5 @@
/* eslint-disable no-underscore-dangle,no-plusplus */
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please keep using current eslint rules?

@potato4d
Copy link

@ngs
At first glance the problem seems to be healed, but is there still anything else?

@ngs
Copy link
Owner

ngs commented Mar 30, 2017

@potato4d @nil1511 I'm awaiting for reply about #17 (comment)

.eslintrc Outdated
"class-methods-use-this": 0
"class-methods-use-this": 0,
"no-plusplus":0,
"no-underscore-dangle":0
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please keep original?

@fandy
Copy link

fandy commented Apr 14, 2017

Any updates on this?

@nil1511
Copy link
Author

nil1511 commented Apr 15, 2017

@ngs Can you check now? Sorry for the delay.

@ngs
Copy link
Owner

ngs commented Apr 15, 2017

@nil1511 Thank you. Test coverage became non-perfect with this PR.

See: https://100-75265088-gh.circle-artifacts.com/0/tmp/circle-artifacts.YP0ThHh/coverage/src/index.js.html

Could you cover non-covered lines/trees?

let newEditorState = EditorState.createWithContent(Draft.convertFromRaw(newRawContentState));
const initialBlockSize = newEditorState.getCurrentContent().getBlockMap().size;
const randomBlockSize = Math.floor((Math.random() * 50) + 1); // random number bettween 1 to 50
for (let i = 0; i < randomBlockSize; i++) { // eslint-disable-line
Copy link
Owner

Choose a reason for hiding this comment

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

Which lint rule did you disable?

Copy link
Author

Choose a reason for hiding this comment

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

no-plusplus rule

src/index.js Outdated
const editorState = getEditorState();
let newEditorState = editorState;
let buffer = [];
for (let i = 0; i < text.length; i++) { // eslint-disable-line
Copy link
Owner

Choose a reason for hiding this comment

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

@nil1511 here as well

Copy link
Author

Choose a reason for hiding this comment

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

no-plusplus rule

@nil1511
Copy link
Author

nil1511 commented Apr 16, 2017

@ngs Can you check the test? I have already updated the description on the first comment.

src/index.js Outdated
const INLINE_STYLE_CHARACTERS = [' ', '*', '_'];

function checkCharacterForState(editorState, character) {
if (INLINE_STYLE_CHARACTERS.indexOf(character) === -1) { return editorState; }
Copy link
Owner

Choose a reason for hiding this comment

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

This line is not covered yet. ref

Copy link
Author

Choose a reason for hiding this comment

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

Removed it. That check was redundant. currently, checkCharacterForState is only called with one of INLINE_STYLE_CHARACTERS. Now code coverage is 100%.

@ngs ngs changed the base branch from master to support-paste April 16, 2017 17:13
@ngs ngs merged commit e6e09c0 into ngs:support-paste Apr 16, 2017
@ngs
Copy link
Owner

ngs commented Apr 16, 2017

@nil1511 Thank you!

@ngs ngs mentioned this pull request Apr 16, 2017
ngs added a commit that referenced this pull request Apr 16, 2017
* Support Pasting markdown

* Fix single line paste

* Reset buffer using assignment

* Move eslint rules to eslintrc

* Revert eslint config

* Added Test case for utils

* Specify disable rule

* Added test case for handlePastedText

* remove redundant check
@ngs
Copy link
Owner

ngs commented Apr 16, 2017

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.

None yet

4 participants