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

Create API to bind list–like UI components with contents of Collection–like classes #5231

Closed
oleq opened this issue May 25, 2016 · 5 comments · Fixed by ckeditor/ckeditor5-ui#49
Assignees
Labels
package:ui type:improvement This issue reports a possible enhancement of an existing feature. type:question This issue asks a question (how to...).
Milestone

Comments

@oleq
Copy link
Member

oleq commented May 25, 2016

A followup of ckeditor/ckeditor5-ui-default#15 (comment).

At the moment List UI component implements own bindings between the collection in the model and collection of child controllers. It might be that some nicer, re–usable API is necessary to automate this process. More use–cases are needed to tell for sure though.

@oleq oleq changed the title Create API to bind list–like components with contents of Collection–like classes Create API to bind list–like UI components with contents of Collection–like classes May 25, 2016
@oleq
Copy link
Member Author

oleq commented Jul 21, 2016

@oleq oleq self-assigned this Jul 25, 2016
@oleq
Copy link
Member Author

oleq commented Jul 25, 2016

I proposed a solution in ckeditor/ckeditor5-ui#49. The idea is that ControllerCollection class accepts optional model collection, controller and view classes. Like this:

const models = new Collection();
const controllers = new ControllerCollection( 'foo', models, FooController, FooView );

// Each model becomes an instance of FooController (+FooView) in the controller collection.
models.add( new Model( { ... } ) );
models.add( new Model( { ... } ) );
console.log( controllers.length == 2 );

// Controller collection is updated as the model is removed.
models.remove( ... );
console.log( controllers.length == 1 );

It solves the all the simple cases like when the all the children of the parent controller are of the same class. It works perfectly for the List component and it's ListItem children.

Sadly, it is not the right solution when there's some diversity among the children of the controller, i.e. it will not work for a toolbar, when there are buttons, dropdowns and separators to be rendered. Even if their models belong to a single Collection, they would be rendered as the same type of component (new ControllerCollection( 'foo', models, FooController, FooView );).

Now the question is: do we want to address more complex cases, like toolbar? And if yes, then how? I think there could be some way to pass the information about which controller class should be used to represent a particular model instance in the collection. There's also a possibility of moving the logic to some generic UI component like ToolbarItem (?), which would then decide if a particular model should be rendered as a button, or maybe some dropdown. Then the controller collection for the toolbar would consist of ToolbarItem instances only.

WDYT?

@Reinmar
Copy link
Member

Reinmar commented Jul 26, 2016

First of all, I'd change the API a bit, to make it more verbose:

this.addCollection( 'foo' ); // See https://github.com/ckeditor/ckeditor5-ui/issues/6
this.collections.get( 'foo' ).setFactory( FooController, FooView );

Which can be simplified even further:

this.addCollection( 'foo' ).setFactory( FooController, FooView );

Next, to handle cases like the toolbar we can make the factory a function:

this.addCollection( 'foo' ).setFactory( ( item ) => {
    if ( item.type == 'button' ) {
        return new Button( item, ButtonView ); // note – we assume in this case that item is a model
    } else {
        ...
    }
} );

Now, as for the assumption that item is a model, it doesn't need to be. We could imagine something like:

toolbarModel.buttons.add(
    boldModel, italicModel,
    { type: 'separator' },
    undoModel, redoModel
);

Which would render as two buttons, a separator and next two buttons (the separator must be an object, unfortunately).

OTOH, I'm still unsure how will be toolbar be configured. E.g. if we want to merge two buttons into one dropdown, how would that be defined? Perhaps simply like that:

const dropdownModel = new Model( {
    items: new Collection()
} );

dropdownModel.items.add( boldModel, italicModel );

toolbarModel.buttons.add(
    dropdownModel
);

However, configuration through config.toolbar must be as simple as possible and must be based on strings because models are not yet available. It seems though that there should be no problem in automatically transforming the below configuration into code like above (which would happen inside the toolbar controller and may be limited to just a few components if strings were used to specify item types):

config.toolbar = [
    {
        type: 'dropdown',
        items: [ 'bold', 'italic' ]
    },
    '|',
    'undo', 'redo'
];

I've written that configuration can be "limited to just a few components if strings were used to specify item types", because the toolbar controller will have to import those components statically. It neither knows about any possible component that could be placed in the toolbar, nor it should import all (for code size reasons). But that's fine I think because the special cases may be handled by operating on the toolbar model directly, not through the configuration.

@Reinmar
Copy link
Member

Reinmar commented Jul 26, 2016

BTW. as for the factory: http://backbonejs.org/#Collection-model

@Reinmar
Copy link
Member

Reinmar commented Jul 26, 2016

And I guess the above makes it for an R-.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-ui Oct 9, 2019
@mlewand mlewand added this to the iteration 2 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. type:question This issue asks a question (how to...). 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. type:question This issue asks a question (how to...).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants