Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

t/1: The first implementation of the decoupled editor #2

Merged
merged 8 commits into from
Mar 13, 2018
Merged

Conversation

oleq
Copy link
Member

@oleq oleq commented Mar 9, 2018

Suggested merge commit message (convention)

Feature: The first implementation of the decoupled editor. Closes ckeditor/ckeditor5#2240. Closes ckeditor/ckeditor5#873.

* @returns {Promise} A promise resolved once the editor is ready.
* The promise returns the created {@link module:editor-decoupled/decouplededitor~DecoupledEditor} instance.
*/
static create( data, config ) {
Copy link
Member

Choose a reason for hiding this comment

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

To avoid extending EditorConfig with additional properties, I think that we could define DecoupledEditorConfig as an extension of EditorConfig and add them there.

* Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

Copy link
Member

Choose a reason for hiding this comment

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

Why does it exist? Maybe a comment here? Or can we remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Especially that I can see something like this:

 [ThemeImporter] Failed to find "/workspace/ckeditor5/packages/ckeditor5-theme-lark/theme/ckeditor5-editor-decoupled/decouplededitor.c

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like a post–copy&paste orphan.

Copy link
Member

Choose a reason for hiding this comment

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

I removed it and all seems to work fine.

</p>

<h2>The toolbar</h2>
<div class="toolbar-container ck-reset_all"></div>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about .ck-reset_all and .ck-reset. Without them the editor doesn't look as it should. This is a pity.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't the editor UI add these classes to the editable and the toolbar automatically?

Copy link
Member

Choose a reason for hiding this comment

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

I pushed (6393bf2) what seems to be a fix for it to me. We may also consider adding this class directly in ToolbarView.

@Reinmar Reinmar merged commit a1950e8 into master Mar 13, 2018
@Reinmar Reinmar deleted the t/1 branch March 13, 2018 10:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants