Opened 4 years ago

Last modified 3 years ago

#13408 review New Feature

Move widget initialization from autoembed to widget repo's method

Reported by: Piotrek Koszuliński Owned by: Szymon Cofalik
Priority: Nice to have (we want to work on it) Milestone:
Component: General Version: 4.5.0
Keywords: Cc:

Description

This is:

		var defaults = typeof widgetDef.defaults == 'function' ? widgetDef.defaults() : widgetDef.defaults,
			element = CKEDITOR.dom.element.createFromHtml( widgetDef.template.output( defaults ) ),
			instance,
			wrapper = editor.widgets.wrapElement( element, widgetDef.name ),
			temp = new CKEDITOR.dom.documentFragment( wrapper.getDocument() );

		temp.append( wrapper );
		instance = editor.widgets.initOn( element, widgetDef );

		if ( !instance ) {
			finalizeCreation();
			return;
		}

Change History (25)

comment:1 Changed 4 years ago by Piotrek Koszuliński

Status: newconfirmed

comment:2 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:3 Changed 4 years ago by Piotrek Koszuliński

Just to clarify - we are talking here about a new method.

comment:4 Changed 4 years ago by Artur Delura

Status: assignedreview

Changes and tests in branch:t/13408.

comment:5 Changed 4 years ago by Piotr Jasiun

Status: reviewreview_failed

New function is over-complicated and unclear.

You return object which, beside the instance, returns wrapperParent which is a temporary documentFragment, which needs to be used at some point later as a parameter of editor.widgets.finalizeCreation... It would be perfect if finalizeCreation would be called automatically and this method return just an instance.

Secondly, the method is called createFromTemplate but there is no template parameter and it is not mentioned in the description. I am not sure if it should really be called this way. Maybe createFromDefinion or simple create would be better? But if the template is needed in the definition to make this method work this should be mentioned in the documentation.

In fact description is very enigmatic: Creates a widget based on template and adds one to repository. Why I can not use simply widget constructor or initOn method? What is the difference?

I rebased the branch on the newest master.

comment:6 Changed 4 years ago by Piotr Jasiun

Owner: Artur Delura deleted
Status: review_failedconfirmed

comment:7 Changed 4 years ago by Szymon Cofalik

Owner: set to Szymon Cofalik
Status: confirmedassigned

comment:8 Changed 4 years ago by Piotrek Koszuliński

Excerpt from my F2F discussion with @scofalik – the code was good and only some polish should be applied to it.

First of all – why there and not in the constructor or initOn()? Because it cannot be merged into some other function. This process includes many phases and for the sake of testability as well as composability and reusability, we should not create one monolithic method.

Then, there's the question about "is it a good API?". Well – it's definitely confusing. First of all – why the container? Why so many strange checks? Unfortunately, all this is needed in so generic API as widgets are. If we knew all these cases from the beginning we would definitely shape it differently, perhaps abstracted it. Right now we need to base on documentation.

So what I would see improved?

  • The name of the method – create() will be enough. It should accept widget definition and startupData.
  • Return value – { container, instance }.
  • Documentation – I will write it during review.

comment:9 Changed 4 years ago by Szymon Cofalik

Status: assignedreview

I've pushed branch:t/13408 with the changes we discussed.

comment:10 Changed 4 years ago by Olek Nowodziński

Status: reviewreview_failed

comment:11 Changed 4 years ago by Szymon Cofalik

Status: review_failedassigned

comment:12 Changed 4 years ago by Szymon Cofalik

Status: assignedreview

Pushed fixed version on branch:t/13408. Sorry for that mistake. I re-run tests on all browsers and they seem fine now.

Last edited 4 years ago by Szymon Cofalik (previous) (diff)

comment:13 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.2CKEditor 4.5.3

@a.nowodzinski - please, leave this the review to me because I have to rethink all the weird cases which may happen during widget initialisation and write docs for this.

PS. Postponing, because this is nothing crucial.

comment:14 in reply to:  13 Changed 4 years ago by Olek Nowodziński

Replying to Reinmar:

@a.nowodzinski - please, leave this the review to me because I have to rethink all the weird cases which may happen during widget initialisation and write docs for this.

PS. Postponing, because this is nothing crucial.

Alright. Enjoy your toys :)

comment:15 Changed 4 years ago by Olek Nowodziński

Milestone: CKEditor 4.5.3CKEditor 4.5.4

comment:16 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.5.4CKEditor 4.5.5

comment:17 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.5CKEditor 4.5.6

comment:18 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.6CKEditor 4.5.7

comment:19 Changed 4 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.7CKEditor 4.5.8

comment:20 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.8CKEditor 4.5.9

comment:21 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.9CKEditor 4.5.10

comment:22 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.10CKEditor 4.5.11

Moving tickets to the next milestone.

comment:23 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.5.11CKEditor 4.6.1

comment:24 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.1CKEditor 4.6.2

Moving to 4.6.2 minor release, as 4.6.1 is mostly about polishing 4.6.0.

comment:25 Changed 3 years ago by Marek Lewandowski

Milestone: CKEditor 4.6.2
Priority: NormalNice to have (we want to work on it)

This ticket has already nice rescheduling streak :D Moving it simply to our todo list, we'll get back to it as we have time, but it's not a crucial one for the time being.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy