Use editor3 internal clipboard to copy entities from open editors #2509

pablopunk · 2018-08-29T12:34:51Z

SDESK-2996

The problem was that tables could not be pasted between editors because draft-js doesn't handle very well pasting custom entities to another editor. This attempts to use the first editor content (via internal clipboard) and insert it directly into the second editor.

If you copy something from an editor with images/tables/etc, refresh the page, and then paste it to another editor this fix won't work as it has no reference to the other editor. I'm thinking of another way of handling this situation, like copying the actual contentState instead of the html but that can be messy and I think it's not possible right now in draft

*My first approach to this issue was handling all paste by our custom html parser. I did implement this and it worked for tables but images/media were kind of tricky so I went for the second fix.

pablopunk · 2018-08-29T12:39:10Z

tsconfig.json

@@ -11,8 +11,8 @@
        "target": "es5",
        "lib": [
            "dom",
-            "es2015"
+            "es2017"


FYI: I needed this to get Object.entries recognized by typescript but I ended up dropping that code. I still think we'll need this in the future.

@tomaskikutis maybe you know, could this commit cause us any problem regarding typescript compilation?

This works for me

"lib": [ "dom", "es2015", "es2016" ]

I think you might need to still include es2015 and es2016 even if you have es2017 included.

@tomaskikutis are you sure? That's weird, I would assume the es2017 standard includes both es2015 and es2016

Try and see, but I think it might not automatically include the features of previous editions of the spec

Well it is working right now just with es2017, the compiler doesn't complain about anything

tomaskikutis · 2018-09-03T06:29:56Z

scripts/core/editor3/components/handlePastedText.ts

-function processPastedHtml(props, html) {
-    const {onChange, editorState, editorFormat} = props;
-    let pastedContent = getContentStateFromHtml(html);
+function insertContentInState(props: any, _pastedContent: ContentState) : DraftHandleValue {


Instead of props, take the actual arguments you need: editorState, editorFormat, onChange. This way function signature becomes cleaner and you can add type annotations without needing to add it for the entire object.

It also makes it easier to understand and refactor the code if you take fewer arguments. For example pasteContentFromOpenEditor which calls this function(insertContentInState) takes props as an argument, and while I'm looking at pasteContentFromOpenEditor I'm thinking "why does it take all props as an argument? And then I see: oh, it passes the entire object to another function, it must require it then I think. In this case, it's only two levels, but when you keep doing it, you end up with 6 levels of passing the entire object when you only need a few params from it which obfuscates your intention and creates dependencies(on the entire object) which should not be there.

Then comes a time when you need to call that function and you have those 3 required values(editorState, editorFormat, onChange), but not the entire object and you think: "I'll just create an empty object with these 3 values and it will work for that function". And then someone modifying the function thinks "I've got the entire object, I can start using another property" - and the errors start coming.

That's probably more context than required, but I saw these things happen in the code and it's quite simple to avoid it.

By the way, if you use props outside a React component, specify what kind props is that. It's obvious while in a React component, but otherwise it looks like _this as an argument which doesn't really mean anything outside of the class.

Yeah you're right. I didn't think about it as I didn't write insertContentInState (just renamed it) but I'm working on it right now while I fix other types. Thanks

tomaskikutis · 2018-09-05T04:02:14Z

package.json

@@ -28,6 +28,7 @@
  ],
  "main": "scripts/index.js",
  "dependencies": {
+    "@types/draft-js": "^0.10.24",


It might not cause any issues, but I'd lock the version.

tomaskikutis · 2018-09-05T04:11:20Z

scripts/core/editor3/components/handlePastedText.ts

@@ -25,7 +20,8 @@ function removeMediaFromHtml(htmlString) : string {
    return element.innerHTML;
 }

-function pasteContentFromOpenEditor(props: any, html: string) : DraftHandleValue {
+function pasteContentFromOpenEditor(
+    html: string, editorState: EditorState, onChange: Function, editorFormat: Array<string>) : DraftHandleValue {


It's alright for now, but try avoiding the use of Function type, since it's very generic. You can define

interface IEditor3props { onChange(editorState: EditorState): void; }

and then access onChange like a object property, so you have the whole function signature, but don't need to typed it entirely.

function functionWhichTakesOnChangeAsAnArgument(onChange: IEditor3props['onChange']) { // ... }

tomaskikutis · 2018-09-05T04:14:27Z

scripts/core/editor3/components/handlePastedText.ts

@@ -34,15 +30,15 @@ function pasteContentFromOpenEditor(props: any, html: string) : DraftHandleValue
            if (internalClipboard) {
                const blocksArray = [];

-                internalClipboard.map((b) => blocksArray.push(b));
+                internalClipboard.forEach((b) => blocksArray.push(b));


Why aren't you passing internalClipboard to createFromBlockArray and instead are making a shallow copy?

tomaskikutis · 2018-09-05T04:15:33Z

scripts/core/editor3/components/handlePastedText.ts

            }
        }
    }

-    return DraftHandleValue.NOT_HANDLED;
+    return 'not-handled';


What's wrong with DraftHandleValue?

tomaskikutis · 2018-09-05T04:19:53Z

scripts/core/editor3/helpers/highlights.ts

@@ -6,6 +6,7 @@ import {getDraftSelectionForEntireContent} from './getDraftSelectionForEntireCon
 import {resizeDraftSelection} from './resizeDraftSelection';
 import {clearInlineStyles} from './clearInlineStyles';
 import {changeSuggestionsTypes, blockStylesDescription, paragraphSuggestionTypes} from '../highlightsConfig';
+import _ from 'lodash';


import {onlyWhatYouNeed} from 'lodash', so we can have a smaller bundle size.

tomaskikutis · 2018-09-05T04:41:22Z

scripts/core/editor3/reducers/find-replace.tsx

@@ -1,4 +1,6 @@
+/* eslint-disable no-unused-vars */ // eslint complains about EditorChangeType not being used


You can use eslint-disable-next-line to avoid having to re-enable rules.

// eslint-disable-next-line no-unused-vars import {EditorChangeType} from 'draft-js'; import {SelectionState, Modifier, EditorState} from 'draft-js';

ioanpocol · 2018-09-06T07:52:29Z

patches/@types/draft-js+0.10.24.patch

@@ -0,0 +1,19 @@
+patch-package


Do we need this file?

@ioanpocol I added a new dependency: patch-package. With it we can modify any package under node_modules and make a patch like this one to fix a bug without waiting for a PR to be merged. This will patch the module after an npm install automatically

pablopunk added the wip label Aug 29, 2018

pablopunk commented Aug 29, 2018

View reviewed changes

pablopunk added 2 commits August 29, 2018 15:43

WIP

8eb3cdb

Pasting tables works between open editors

8bbb853

pablopunk force-pushed the internal-clipboard branch from 0bbf8c6 to 8bbb853 Compare August 29, 2018 14:39

petrjasek added this to the 1.25 milestone Aug 30, 2018

Add handled value enum and some types in functions

74efd48

pablopunk added need review wip and removed wip need review labels Aug 30, 2018

tomaskikutis reviewed Sep 3, 2018

View reviewed changes

pablopunk added 4 commits September 4, 2018 15:44

Add types for draft-js

b1407ca

Refactor paste functions with typed arguments

d3ecbc1

Install patch-package and fix draft-js types export

ab0ed09

Don't use Editor3 props as arguments

50f25d3

pablopunk removed the wip label Sep 4, 2018

Disable eslint no-unused-vars when importing type (fix tests)

733b6eb

tomaskikutis reviewed Sep 5, 2018

View reviewed changes

Minor fixes

9561daa

tomaskikutis approved these changes Sep 5, 2018

View reviewed changes

ioanpocol reviewed Sep 6, 2018

View reviewed changes

ioanpocol approved these changes Sep 6, 2018

View reviewed changes

pablopunk added 2 commits September 6, 2018 11:59

Merge remote-tracking branch 'upstream/master' into internal-clipboard

0a13240

Fix tslint issues

a37dfa7

pablopunk merged commit d22909d into superdesk:master Sep 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use editor3 internal clipboard to copy entities from open editors #2509

Use editor3 internal clipboard to copy entities from open editors #2509

pablopunk commented Aug 29, 2018 •

edited

Loading

pablopunk Aug 29, 2018

pablopunk Aug 29, 2018

tomaskikutis Aug 30, 2018

pablopunk Aug 30, 2018

tomaskikutis Aug 30, 2018

pablopunk Aug 30, 2018

tomaskikutis Sep 3, 2018

pablopunk Sep 3, 2018

tomaskikutis Sep 5, 2018

tomaskikutis Sep 5, 2018

tomaskikutis Sep 5, 2018

tomaskikutis Sep 5, 2018

tomaskikutis Sep 5, 2018

tomaskikutis Sep 5, 2018

ioanpocol Sep 6, 2018

pablopunk Sep 6, 2018

ioanpocol Sep 6, 2018

		@@ -1,4 +1,6 @@
		/* eslint-disable no-unused-vars */ // eslint complains about EditorChangeType not being used

Use editor3 internal clipboard to copy entities from open editors #2509

Use editor3 internal clipboard to copy entities from open editors #2509

Conversation

pablopunk commented Aug 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pablopunk commented Aug 29, 2018 •

edited

Loading