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

Controller collection should not be bind with the view by name #5261

Closed
pjasiun opened this issue Oct 19, 2016 · 4 comments
Closed

Controller collection should not be bind with the view by name #5261

pjasiun opened this issue Oct 19, 2016 · 4 comments
Labels
package:ui type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@pjasiun
Copy link

pjasiun commented Oct 19, 2016

As far as I understand controller collections bind with view regions by the same name.

I think it is wrong, because of two reasons.

First: it looks like magic, I was looking for some code which binds this collection for a while before I understand that they are bind behind the scene, by names.

Second: I understand controllers as code which glue two separate APIs: model and view. Expecting that there will be a collection with the same names looks wrong.

@Reinmar
Copy link
Member

Reinmar commented Oct 19, 2016

There's one more thing wrong here – collections of controller collections. This is super confusing.

We should try to achieve something like:

class SomeController extends Controller {
    constructor() {
       this.items = new ControllerCollection( this.view.items ); // Maps ctrl items into view items.

       // or, if the parent controller must know about its child collection (and its children):
       this.addCollection( 'items' );

What's more, since the SomeController#items would often be mapped into some other collection (model), this could perhaps even look like this:

// or this.bind()
this.map( this.items ).to( this.view.items, ( itemModel ) => {
    return new Controller( itemModel, new ItemView() );
} );

Of course, these are only some wild ideas. We need to understand whether they can be translated into something useful and something that can work.

@pjasiun
Copy link
Author

pjasiun commented Oct 19, 2016

Yes, I can confirm t that collection of the controllers collections in very confusing and calling:

collection.add( 'items', presence );

Instead:

collection.items.add( presence );

Is very confusing.

@Reinmar
Copy link
Member

Reinmar commented Oct 19, 2016

collection.items.add( presence );

I think you meant:

controller.items.add( presence );

@Reinmar
Copy link
Member

Reinmar commented Nov 9, 2016

Fixed by ckeditor/ckeditor5-ui#100.

@Reinmar Reinmar closed this as completed Nov 9, 2016
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-ui Oct 9, 2019
@mlewand mlewand added this to the iteration 5 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:ui labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:ui type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

3 participants