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

Move ViewCollection.bind() to utils.Collection #4935

Closed
pjasiun opened this issue Feb 7, 2017 · 6 comments · Fixed by ckeditor/ckeditor5-utils#131
Closed

Move ViewCollection.bind() to utils.Collection #4935

pjasiun opened this issue Feb 7, 2017 · 6 comments · Fixed by ckeditor/ckeditor5-utils#131
Assignees
Labels
package:utils type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@pjasiun
Copy link

pjasiun commented Feb 7, 2017

Since it's a very useful feature to bind two collections, especially if you can do custom mapping, it should be moved to the utils.Collection to let users bind any collections, not only ViewCollection.

Note that you can bind any observable variable, but not any observable collection, what is inconsistent.

The problem is that the binding checks if the parameter is the instance of View to check if it should be used or if the parameter is a creator function (https://github.com/ckeditor/ckeditor5-ui/blob/6b7fd4adfe6ee99fbdf84bfc4940f10ed78493b8/src/viewcollection.js#L227-L246). To solve it we could split the as function to separate function, what would make the code simpler and less magic:

collection1.bindTo( collection2 ).byWrappingInto( MyViewClass );
collection1.bindTo( collection2 ).using( ( object ) => object.propery );

Note that in the future we might introduce another popular mapping helper, like for the last line from the example above:

collection1.bindTo( collection2 ).withProperty( 'property' );
@oleq
Copy link
Member

oleq commented Mar 1, 2017

It's probably more a matter of

collection1.bindTo( collection2 ).using( ( object ) => object.propery );

where for UI, it would be

collection1.bindTo( collection2 ).using( viewFactoryInstance );

Current implementation is straightforward. I think it's an easy thing to do.

@pjasiun
Copy link
Author

pjasiun commented Mar 1, 2017

But binding by wrapping might be a common case not only for UI, as @wwalc and @scofalik suggested.

@oleq
Copy link
Member

oleq commented Mar 1, 2017

I'm not sure what's the purpose of byWrappingInto. For me there are 2 real use–cases:

  1. Binding a property like
collection1.bindTo( collection2 ).using( ( object ) => object.propery );
  1. Binging as a factory i.e.
collection1.bindTo( collection2 ).using( ( object ) => new AnotherClass( object ) );

which as good as

collection1.bindTo( collection2 ).as( AnotherClass );

Is there anything more than that?

@pjasiun
Copy link
Author

pjasiun commented Mar 10, 2017

For me that's fine.

@pjasiun
Copy link
Author

pjasiun commented Mar 10, 2017

By the way: it would be nice to have the two-way binding of collections. Will it work out of the box, or we need a separate ticket for it?

@oleq
Copy link
Member

oleq commented Mar 10, 2017

@pjasiun I'm not sure what outcome would you expect when the binding is used as a factory.

But anyway, we considered it in the UI https://github.com/ckeditor/ckeditor5-ui/issues/110.

@oleq oleq self-assigned this Mar 10, 2017
pjasiun referenced this issue in ckeditor/ckeditor5-utils Mar 14, 2017
Feature: `Collectio.bindTo` method now is not only available in the `ViewCollection` but in all `Collection`s. Closes #125.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-utils Oct 9, 2019
@mlewand mlewand added this to the iteration 9 milestone Oct 9, 2019
@mlewand mlewand added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). package:utils labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:utils type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants