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

How to create editor without replacing an existing element #2882

Closed
Reinmar opened this issue Feb 13, 2017 · 12 comments · Fixed by ckeditor/ckeditor5-core#129
Closed

How to create editor without replacing an existing element #2882

Reinmar opened this issue Feb 13, 2017 · 12 comments · Fixed by ckeditor/ckeditor5-core#129
Assignees
Labels
package:core type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Feb 13, 2017

@oskarwrobel asked me today how to create the classic editor with data which he stores in JS. Well, turns out that you need to create a fork of the ClassicEditor.

We've never thought about this enough I think. The assumption that the editor is replacing an existing element can't be generalised. It may be true in sth like 75%, making the editor hard to use in 25% cases.

We need to design the creators in a way that you'll be able to either replace an existing element or create the editor with your own data.

First of all, this means that the concept of "editor element" needs to be revisited. An editor may not have an element if it didn't replace any. So the StandardEditor#element property and updateEditorElement/loadDataFromEditorElement() methods will be optional and perhaps should be removed totally.

Second, we need to define a more general create() method. The first idea which came to my mind was that create() could have two modes – if called with an element it would replace it, if called with a string, it would create an editor with that data.

HOWEVER, I'm a bit worried that we cannot allow creating an editor detached from the DOM. I'm not entirely sure that it's going to work correctly. From my past experience, it may through weird errors.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 13, 2017

RFC for how to shape the API of the base class(es) and the concrete editor classes so they are clean and easily usable in as many scenarios as possible.

@fredck
Copy link
Contributor

fredck commented Feb 13, 2017

It would be great to have create( element, config ) and create( data, config ).

It is as well ok to have editor.element and editor. updateEditorElement() enabled just in the first case and the automatic update of the element on editor destroy.

@Reinmar
Copy link
Member Author

Reinmar commented Mar 23, 2017

OK, so let's do it. I've just had to change a couple of tests all across our code base because ClassicEditor() replaces the element that it gets, which breaks if that element is not in the DOM yet. So that would be another thing to improve – if the editor is not in the DOM yet, use it as a source of the data, but don't try to replace it.

@pjasiun
Copy link

pjasiun commented Feb 7, 2018

It is closed by ckeditor/ckeditor5-core#116. Now, base editor class does not require element nor itscreate method. All element management is a utility which you can use if you need something similar to the classic editor but you are not obligated to do so. Also, all essential configuration is moved to the editor class so implementing a custom editor on top of it is much simpler.

We can still consider changes in ClassicEditor, so you can create it without any element, but it should be a specific ticket in ClassicEditor because it does not make sense to all types of the editor.

@Reinmar
Copy link
Member Author

Reinmar commented Feb 7, 2018

I'm unsure about closing this ticket. It's not only about the ClassicEditor. Other editors might need the same kind of behaviour – creating the whole DOM structure rather than "upcasting" an existing element.

I'll reopen this ticket because the core of this issue is still pending and I'd like to track it in one place. But it's not iteration 14 anymore.

@Reinmar Reinmar reopened this Feb 7, 2018
@Reinmar
Copy link
Member Author

Reinmar commented Jun 6, 2018

Unfortunately, we need to do bigger changes here (see the PRs linked above to understand "bigger than what").

Current state

editor/ElementApi interface adds the Editor#element property and the Editor#updateElement() method.

What the PRs change

If, suddenly, the Editor#element property may hold the editor UI's main element, then Editor#updateElement() becomes invalid. We can force disabling it but to do that we'd need to remember internally what was passed to create(). But why internally? Something's fishy here.

The conclusion is simple – we cannot use one property (editor.element) for two things. We need two properties:

  • one for the editor's source element (the one passed to create()),
  • one for the editor's main UI element.

Solutions

Now, the problem is that we called the first one editor.element. In the past we had the concept of "editor element", but when I look at this today, "editor element" means for me the element which introduces the UI. So, what I'd prefer is this:

  • renaming editor.element to editor.sourceElement and using the "source element" term in .create() and other places.
  • introducing editor.element (by EditorWithUI interface) and pointing it to the editor's main UI element.

But this means a breaking change. So, the alternative is this:

  • leaving editor.element as it is,
  • introducing editor.uiElement.

I strongly prefer the first option because it seems cleaner. WDYT?

@jodator
Copy link
Contributor

jodator commented Jun 6, 2018

Well the editor.element combined with editor.uiElement looks so meaningless to me (I don't have big history with the CKEditor internals). Looking at other parts of API, maybe mostly on the UI/View it's common to have the DOM element associated with some View be as View#element so it makes more sense to take first option and introduce editor.sourceElement (the name could be also somewhat better - but I don't have better ideas then editor.inputElement... so it's minor note only).

tl;dr: first option 👍.

@scofalik
Copy link
Contributor

scofalik commented Jun 6, 2018

Option number 1. Seems more reasonable.

@oskarwrobel
Copy link
Contributor

I'm for option 1. I remember I had a problem to understand what Editor#element is because I expected it's a UI element.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 7, 2018

Perfect, thanks. That's the feeling I had too but it's hard to break legacy :)

@tgroshon
Copy link

I’m very interested in this because I understand it to be important for the React bindings.

I’m curious; what is the current status? I see a bunch of in-flight PRs in this repo and the build repos that have been around for 2-4 weeks. It seems close yes?

@Reinmar
Copy link
Member Author

Reinmar commented Jun 25, 2018

Hi! The first attempt at making this change was pretty close to be merged 2 weeks ago, but then I realised that the resulting API will be confusing. We decided to make broader changes (rename editor.element to editor.sourceElement and reintroduce editor.element with a different meaning) and PRs for that are ready but this is a breaking change, so needed to postpone it. We wanted to avoid making breaking changes in the last release because we also planned other breaking changes for the upcoming release (which should be ready in ~2-3 weeks).

BTW, we also work on https://github.com/ckeditor/ckeditor5-react and will release it in ~3 weeks (once we'll unblock it by releasing the changes from this ticket).

Reinmar referenced this issue in ckeditor/ckeditor5-core Jul 3, 2018
Feature: Introduced the `#element` property to the `EditorWithUI` interface. The `#element` property from the `ElementApi` interface has been renamed to `#sourceElement`. Most editors implement both interfaces, which ultimately means that the old `editor.element` property is now called `editor.sourceElement` and there's a new `editor.element` property with a new meaning. Closes #64.

BREAKING CHANGE: The `editor.element` property was renamed to `editor.sourceElement`.

BREAKING CHANGE: The `editor.updateElement()` method was renamed to `editor.updateSourceElement()`.
Reinmar referenced this issue in ckeditor/ckeditor5-editor-classic Jul 3, 2018
Feature: Editor can now be created with initial data passed to the `create()` method. Closes #72.

BREAKING CHANGE: The `ClassicEditor#element` property was renamed to `ClassicEditor#sourceElement`. See ckeditor/ckeditor5-core#64.
Reinmar referenced this issue in ckeditor/ckeditor5-editor-inline Jul 3, 2018
Feature: Editor can now be created with initial data passed to the constructor. Closes #37.

BREAKING CHANGE: The `InlineEditor#element` property was renamed to `InlineEditor#sourceElement`. See ckeditor/ckeditor5-core#64.
Reinmar referenced this issue in ckeditor/ckeditor5-editor-decoupled Jul 3, 2018
Other: Aligned `DecoupledEditor` to changes in the `EditorWithUI` and `ElementApi` interfaces.

BREAKING CHANGE: `DecoupledEditor#element` was renamed to `DecoupledEditor#sourceElement`. See ckeditor/ckeditor5-core#64.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-core Oct 9, 2019
@mlewand mlewand added this to the iteration 19 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:core labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:core type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants