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

A bit confusing editor initialization. #2245

Closed
jodator opened this issue Mar 16, 2018 · 13 comments · Fixed by ckeditor/ckeditor5-editor-decoupled#11
Closed

A bit confusing editor initialization. #2245

jodator opened this issue Mar 16, 2018 · 13 comments · Fixed by ckeditor/ckeditor5-editor-decoupled#11
Assignees
Labels
package:editor-decoupled type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@jodator
Copy link
Contributor

jodator commented Mar 16, 2018

This is probably due to lack of docs or just that other

The confusing part might be with how to set data for editing in the DecoupledEditor.create() method as it kinda works differently then with other editors.

Th consfusing part was that editableContainer cannot be used to init data to edit (despite the fact that there's a param for feeding the editor with data).

Also the confusing part is that if there's anything in the editableContainer it will be around editable area but will not be editable.

selection_030

Now the confusion probably came from that other editors used some container as both editable container and to feed data to the editor.

@oleq
Copy link
Member

oleq commented Mar 16, 2018

What do you propose to make it less confusing?

Given the specific nature and the purpose of this editor, the API we created makes a lot of sense to me, TBH.

@jodator
Copy link
Contributor Author

jodator commented Mar 16, 2018

@oleq Yeah but the very first user had problems with using it - that's why I'm reporting this.

@Reinmar
Copy link
Member

Reinmar commented Mar 16, 2018

The API made a lot of sense for me too. But I'm afraid that we might've fixated on the purpose of this editor so much that we missed how its API might be confusing.

I've heard from two guys today that they don't understand how to enable this editor. And this can be for two reasons:

  • the document is unclear,
  • it works so differently from other editors that you get confused by that.

Therefore, we should start from checking whether the documentation stresses the fact what happens when you initialise this editor.

However, we should also think about making it work similarly to other editors. I've been discussing this with @wwalc and have some ideas, but, TBH, it'll be tricky.

First of all – specifying a string as a data source is unnecessarily problematic in some cases. When rendering a website it's usually easier to put the content into some HTML element (div or textarea). So, we might add the ability to initialise this editor on an element, like all other editors:

DecoupledEditor
    .create( document.querySelector( '#data-source' ) )
    ...

However, I haven't named this element data-source by coincident. The decoupled editor can only treat this element as a data source – it should hide it and load data from it to the editor. But it cannot render itself there. You still need to specify config.editableContainer and config.toolbarContainer or insert these elements manually.

So, it will still be a bit confusing. The above code will result in... hiding the #data-source element and nothing more. Is it ok? I guess not really. But can we do more? Not really, as well. The decoupled editor cannot inject itself anywhere.

WDYT, guys?

@jodator
Copy link
Contributor Author

jodator commented Mar 16, 2018

I think that making them separate (editing element and data source element) will be confusing as well.

I think that an expected thing is to have "editable area with an existing HTML content" where the "existing HTML content" was on page.

But then thinking on how such editor might be used is:

I have areas where I want to create editor:
a) toolbar area
b) editable/editor area

so those have to be defined.

Then either I have existing content - let's say in that editable/editor area or from other source - let it be JS string. Then I'd like to provide this data to the editor upon creating or just right after.

Looking how I believe it would be used i think that HTML data to load should be taken either from the editable area or passed as a parameter (or alternatively set later on to the editor).

The third element #data-source is redundant here - also I don't see a point of loading data from this container if one could pass it as parameter.

Maybe some explicit errors would help. Ie in situation of loading data from existing editable container and passing a initial data at the same time the editor should throw?

@oleq
Copy link
Member

oleq commented Mar 19, 2018

I think that making them separate (editing element and data source element) will be confusing as well.

That's a good point too.

So, it will still be a bit confusing. The above code will result in... hiding the #data-source element and nothing more. Is it ok? I guess not really. But can we do more? Not really, as well.

Should it set data back to the data-source, e.g. on form submit?

@jodator
Copy link
Contributor Author

jodator commented Mar 19, 2018

Should it set data back to the data-source, e.g. on form submit?

I get the feeling that it depends:

  • if the editable element is a <textarea> then probably it should be in some sort of form then probably the editor should update that <textarea>
  • if the editable is <div> or whatever with contentedible the I'd tread that as single page application with data loaded/saved via JavaScript. In such case probably the developer would need to get the data from the editor on some external event.

@m-kr
Copy link

m-kr commented Mar 20, 2018

I agree with @jodator about confusing init. From my perspective - someone who doesn't make anything with CKEditor, besides using builds to create demos...

All editors are initialized by InlineEditor.create( inlineElement, config ) thus I expect that every other will be inited like that: the first argument means the element which is used as the data source and in the same time as editable area, the second as editor config.

This type of editor is different, the toolbar may be in a different place than editable area. When I'm aware of that, I think that the first parameter should be checked whether this is a string or Node element, if string then use it as the source of data, if Node then as editable area and its content as the data source. This behavior can be overwritten by editableContainer in the config.

What was the most confusing for me, is a fact that when I provide a string with editable content, then it is appended to the editableContainer, but it's not cleared before. My data is located after existed content. I don't see a case when this is useful, IMO container's content should be replaced by data from the first argument.

@Reinmar
Copy link
Member

Reinmar commented Mar 20, 2018

Maybe we should remove both config options – editableContainer and toolbarContainer?

There would be two ways to initialize the editor then:

DecoupledEditor
	.create( elementToBeConvertedToEditable ) // like in InlineEditor!
	.then( editor => {
		someContainer.appendChild( editor.ui.toolbar.element );
	} );

DecoupledEditor
	.create( dataString )
	.then( editor => {
		someContainer.appendChild( editor.ui.toolbar.element );
		someOtherContainer.appendChild( editor.ui.editable.element );
	} );

I think that this is very similar to other editors. WDYT?

@m-kr
Copy link

m-kr commented Mar 20, 2018

LGTM, plus dataString should replace someOtherContainer content.

@Reinmar
Copy link
Member

Reinmar commented Mar 20, 2018

It won't replace it in the code sample I showed above – the editable will be appended to someOtherContainer. But the thing is that the developer will be the one to make the decision.

@oleq
Copy link
Member

oleq commented Mar 22, 2018

Can I assume that everyone agrees with https://github.com/ckeditor/ckeditor5-editor-decoupled/issues/10#issuecomment-374545217? I'd like to implement it ASAP.

@oleq
Copy link
Member

oleq commented Mar 22, 2018

@jodator WDYT?

@jodator
Copy link
Contributor Author

jodator commented Mar 22, 2018

@oleq If @m-kr is OK with this we're probably gonna be safe now :) so 👍 .

Reinmar referenced this issue in ckeditor/ckeditor5-editor-decoupled Mar 29, 2018
Other: Allowed the editable element to be passed into `DecoupledEditor.create()`. Removed `config.toolbarContainer` and `config.editableContainer`. Closes #10. Closes ckeditor/ckeditor5#912.

BREAKING CHANGE: The config options `config.toolbarContainer` and `config.editableContainer` have been removed. Please refer to the `DecoupledEditor` class API documentation to learn about possible methods of bootstrapping the UI.
Reinmar referenced this issue in ckeditor/ckeditor5-build-decoupled-document Mar 29, 2018
Other: Updated the build to the latest `DecoupledEditor` class API (see ckeditor/ckeditor5-editor-decoupled#10).

BREAKING CHANGE: The config options `config.toolbarContainer` and `config.editableContainer` have been removed. Please refer to the `DecoupledEditor` class API documentation to learn about possible methods of bootstrapping the UI.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-editor-decoupled Oct 8, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:editor-decoupled labels Oct 8, 2019
@mlewand mlewand added this to the iteration 15 milestone Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:editor-decoupled type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
5 participants