Allow to pass initial data to the editor constructor #73

szymonkups · 2018-05-15T15:19:07Z

Suggested merge commit message (convention)

Feature: Editor can be created with initial data passed to the constructor. Closes ckeditor/ckeditor5#2230.

BREAKING CHANGE: ClassicEditor#element is now available as ClassicEditor#sourceElement. See ckeditor/ckeditor5#2882.

Requires: ckeditor/ckeditor5-core#129

coveralls · 2018-05-15T15:23:58Z

Coverage decreased (-6.3%) to 93.75% when pulling 46d2f71 on t/72 into ae98cfd on master.

pomek

I improved a little the manual test. Except that, everything looks and works fine.

Reinmar · 2018-06-06T13:47:33Z

R- due to https://github.com/ckeditor/ckeditor5-core/issues/64#issuecomment-395069721.

Reinmar · 2018-06-11T09:11:22Z

src/classiceditor.js

+	 * @param {HTMLElement|String} elementOrData The DOM element that will be the source for the created editor
+	 * or editor's initial data.
+	 *
+	 * If an element is passed, then it contents will be automatically


its contents

Reinmar · 2018-06-11T09:11:59Z

src/classiceditor.js

+	 *
+	 * If an element is passed, then it contents will be automatically
+	 * {@link module:editor-classic/classiceditor~ClassicEditor#setData loaded} to the editor on startup
+	 * and  the editor element will replace the passed element in the DOM (the original one will be hidden and editor


"editor element" -> link to the new property

Reinmar · 2018-06-11T09:12:52Z

src/classiceditor.js

+	 * and  the editor element will replace the passed element in the DOM (the original one will be hidden and editor
+	 * will be injected next to it).
+	 *
+	 * Moreover, the data will be set back to the original element once the editor is destroyed and


original => source

…itorWithUI interface (added editor.element property).

Reinmar · 2018-06-26T21:27:48Z

src/classiceditor.js

+	/**
+	 * An HTML element that represents the whole editor (with UI). See the {@link module:core/editor/editorwithui~EditorWithUI} interface.
+	 *
+	 * @returns {HTMLElement|null}


Can it be a null here? At what point it's a null?

Reinmar · 2018-06-26T21:28:11Z

src/classiceditor.js

@@ -79,15 +83,24 @@ export default class ClassicEditor extends Editor {
 		attachToForm( this );
 	}

+	/**
+	 * An HTML element that represents the whole editor (with UI). See the {@link module:core/editor/editorwithui~EditorWithUI} interface.


Do we have to redefine this API doc at all?

Reinmar · 2018-06-26T21:32:09Z

src/classiceditor.js

 						editor.fire( 'uiReady' );
 					} )
 					.then( () => editor.editing.view.attachDomRoot( editor.ui.view.editableElement ) )
-					.then( () => editor.data.init( getDataFromElement( element ) ) )
+					.then( () => {


It'd be easier to read if it was written like this:

const initialData = isElement( sourceElementOrData ) ? getDataFromElement( sourceElementOrData ) : sourceElementOrData; editor.data.init( initialData );

The point is – what differs these two scenarios isn't how or whether you call editor.data.init(). It's how you retrieve this data and from where.

Note that the code you committed isn't equal to what I proposed:

editor.data.init( isElement( sourceElementOrData ) ? getDataFromElement( sourceElementOrData ) : sourceElementOrData );

In this case, we lose the information what do we call init() with. It makes reading the code harder.

BTW, editor.data.init() may return a promise so this change breaks the promise chain – make sure to add a test for that.

Reinmar · 2018-06-26T21:34:00Z

src/classiceditor.js

+							editor.data.init( getDataFromElement( editor.sourceElement ) );
+						} else {
+							editor.data.init( sourceElementOrData );
+							editor.sourceElement = editor.ui.view.element;


I think it's a mistake. Probably a leftover.

BTW, how did tests pass with this? There must be a missing test.

pomek · 2018-06-29T10:16:59Z

@Reinmar, would you like to check it once again?

Reinmar · 2018-06-29T11:57:08Z

Here you are :D

pomek · 2018-07-02T06:31:22Z

Fixed.

Reinmar · 2018-07-02T13:14:58Z

I'll be pedantic – it's still not the same as what I proposed. I named the variable initialData on purpose to make it clear that it's not just any data, but the data you load initially. Compared to a simple data it brings more information on what's happening. Just like, not every key in an object is just a key. They are command names, element names, plugin names, keystrokes, whatever.

It's not critical here. It's not even a place when one may have some bigger doubts. But did you think about this when changing the name? Cause that should've been intentional and I don't think it was.

pomek · 2018-07-02T13:17:40Z

When the name is initialData, length of the line is bigger than 140 and it requires additional eslint rules... That's the reason why I'm trying "smuggle" other name.

Reinmar · 2018-07-02T14:18:29Z

That is the worst possible reason ;) Try formatting that piece of code differently (split into multiple lines if needed).

szymonkups added 3 commits May 7, 2018 15:47

Allow to pass data to the constructor instaead of element to replace.

3015814

Added tests checking data initialization.

5af49aa

Improved documentation.

633b5d3

szymonkups requested a review from Reinmar May 15, 2018 15:19

szymonkups and others added 4 commits May 15, 2018 18:49

Improved manual test with data initialization.

f38aec7

Removed unused CSS class.

3232bfb

Appending editor to the container in data initialization manual test.

8a7ea54

Minor improvements for the manual test.

ffdede7

pomek approved these changes Jun 5, 2018

View reviewed changes

Reinmar mentioned this pull request Jun 5, 2018

Change the name to @ckeditor/ckeditor5-react and publish first dev preview ckeditor/ckeditor5-react#5

Closed

6 tasks

Reinmar added 2 commits June 6, 2018 13:01

Merge branch 'master' into t/72

216cd88

Docs: Improved API docs for the create() method.

a9497d2

Reinmar added the status:review- label Jun 6, 2018

Reinmar reviewed Jun 11, 2018

View reviewed changes

Renamed editor.element to editor.sourceElement and implemented the Ed…

674c3b0

…itorWithUI interface (added editor.element property).

pomek removed the status:review- label Jun 13, 2018

pomek mentioned this pull request Jun 13, 2018

Changes in EditorWithUI and ElementApi interfaces ckeditor/ckeditor5-core#129

Merged

Reinmar reviewed Jun 26, 2018

View reviewed changes

Reinmar added the status:review- label Jun 26, 2018

Kamil Piechaczek added 3 commits June 29, 2018 10:56

Simplified the code and docs. Added more tests.

51cdeee

Merge branch 'master' into t/72

22b6f4f

Merge branch 'master' into t/72

4a13075

pomek removed the status:review- label Jun 29, 2018

Reinmar added the status:review- label Jun 29, 2018

Fixed broken promises chain (editor.data.init() returns a promise).

c210e56

pomek removed the status:review- label Jul 2, 2018

Kamil Piechaczek and others added 2 commits July 3, 2018 08:33

Used better variable name.

46d2f71

More API docs polish.

6df6ba9

Reinmar merged commit 09cebc6 into master Jul 3, 2018

Reinmar deleted the t/72 branch July 3, 2018 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to pass initial data to the editor constructor #73

Allow to pass initial data to the editor constructor #73

szymonkups commented May 15, 2018 •

edited by pomek

Loading

coveralls commented May 15, 2018 •

edited

Loading

pomek left a comment

Reinmar commented Jun 6, 2018

Reinmar Jun 11, 2018

Reinmar Jun 11, 2018

Reinmar Jun 11, 2018

Reinmar Jun 26, 2018

Reinmar Jun 26, 2018

Reinmar Jun 26, 2018 •

edited

Loading

Reinmar Jun 26, 2018 •

edited

Loading

Reinmar Jun 29, 2018

Reinmar Jun 29, 2018

Reinmar Jun 26, 2018

Reinmar Jun 26, 2018

pomek commented Jun 29, 2018

Reinmar commented Jun 29, 2018

pomek commented Jul 2, 2018

Reinmar commented Jul 2, 2018

pomek commented Jul 2, 2018

Reinmar commented Jul 2, 2018

Allow to pass initial data to the editor constructor #73

Allow to pass initial data to the editor constructor #73

Conversation

szymonkups commented May 15, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

coveralls commented May 15, 2018 • edited Loading

pomek left a comment

Choose a reason for hiding this comment

Reinmar commented Jun 6, 2018

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

Reinmar Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Reinmar Jun 26, 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

pomek commented Jun 29, 2018

Reinmar commented Jun 29, 2018

pomek commented Jul 2, 2018

Reinmar commented Jul 2, 2018

pomek commented Jul 2, 2018

Reinmar commented Jul 2, 2018

szymonkups commented May 15, 2018 •

edited by pomek

Loading

coveralls commented May 15, 2018 •

edited

Loading

Reinmar Jun 26, 2018 •

edited

Loading

Reinmar Jun 26, 2018 •

edited

Loading