Removed StandardEditor class #116
Conversation
src/editor/editor.js
Outdated
| * methods and properties. | ||
| * | ||
| * See also {@link module:core/editor/standardeditor~StandardEditor}. | ||
| * Editors implementation (like {@link module:editor-classic/classiceditor~ClassicEditor Classic Editor} or |
There was a problem hiding this comment.
"Classic editor" or simply remove this (so it will say ClassicEditor).
src/editor/utils/elementinterface.js
Outdated
| * @readonly | ||
| * @member {HTMLElement} #element | ||
| */ | ||
| element: null, |
There was a problem hiding this comment.
This should not be a part of the mixin. It will go into the prototype anyway. It's simply unnecessary here.
There was a problem hiding this comment.
into the prototype anyway.
You mean to the interface?
There was a problem hiding this comment.
By "will go" I meant that this property will be added to SomeEditor.prototype. It makes no sense there. Properties (physically) should only be added in instances. The property documentation, though, should go to the interface, together with other docs.
src/editor/utils/datainterface.js
Outdated
| * @mixin DataInterface | ||
| * @implements module:core/editor/utils/datainterface~Data | ||
| */ | ||
| const DataInterface = { |
There was a problem hiding this comment.
So far we called all mixins "mixins". Like ObservableMixin which implements the Observable interface. So we didn't use the word Interface.
Also, a simple Data interface is kinda confusing. Editor, by implementing the Data interface becomes a data itself? The name needs to be clearer.
I'd propose something like:
EditorDataApiinterfaceEditorDataApiMixinmixin
There was a problem hiding this comment.
I'm fine with EditorDataApiMixin. However, note that these are in the editor/utils file and will be imported in the editor implementation class, so I believe it will be clear for everybody who imports it that, use it or see it there that it's about DataApi for the editor, not for something else. This is why DataApiMixin is fine for me as well.
There was a problem hiding this comment.
👍 for DataApiMixin. This will be consistent with our approach to not duplicate module's path in class names.
src/editor/utils/datainterface.js
Outdated
| this.data.set( data ); | ||
| }, | ||
|
|
||
| /** |
There was a problem hiding this comment.
Definition of these should go into the interface, not to the mixin. The interface defines the API. The mixin implements it.
src/editor/utils/elementinterface.js
Outdated
| * @mixin ElementInterface | ||
| * @implements module:core/editor/utils/elementinterface~Element | ||
| */ | ||
| const ElementInterface = { |
There was a problem hiding this comment.
All the same notes as in case of the EditorDataApi. Here we could have:
EditorElementMixinmixinEditorElementinterface
But, just like with the "data" stuff, I don't like the fact that editor doesn't become an editor element but it gets an API of such type, so (however I don't like the length of this) the following seems more correct:
EditorElementApiMixinEditorElementApi
PS. API ~= Interface, but I use API because we'd end up with EditorElementInterfaceMixin and EditorElementInterface. Unless... we'd decide that we can omit Interface suffix, but then we end up with an also ugly EditorElement interface.
In other words, I chose "API" because it reads well: "My editor implements the editor element API". While this makes no sense at all: "My editor implements editor element".
|
Ok guys, I think everything is fixed. |
|
Changes Unknown when pulling 5efdaec on t/115 into ** on master**. |
Suggested merge commit message (convention)
Fix: Removed
StandardEditorclass in favor ofDataInterfaceandElementInterfacemixins. AddedEditorWithUIinterface. Closes ckeditor/ckeditor5#2930. Closes ckeditor/ckeditor5#2931. Closes ckeditor/ckeditor5#303.BREAKING CHANGE:
StandardEditorclass is removed. UseEditorclass withDataInterfaceandElementInterfacemixins.Constelation: https://github.com/ckeditor/ckeditor5/tree/t/ckeditor5-core/115
Requires: