Added support for initializing ViewCollection and BodyCollection items using a constructor #545

mlewand · 2020-02-27T10:06:49Z

Suggested merge commit message (convention)

Feature: Added support for initializing ViewCollection and BodyCollection items using a constructor. See ckeditor/ckeditor5#6319.

The View.createCollection() method also accepts an iterator of views.

MAJOR BREAKING CHANGE: ViewCollection no longer has the locale property.

MAJOR BREAKING CHANGE: ViewCollection's constructor no longer takes the locale as a parameter.

Additional information

This PR was mostly based on #524 with some notable changes:

Back then we could have removed locale from all collections. Today locale is required by one Collection subclass - BodyCollection (it uses it to determine text direction). So it is only BodyCollection that contains this property.
ViewCollection / BodyCollection accepts any iterable of views, not just an array.

This PR requires a PR from utils to be working: ckeditor/ckeditor5-utils#326

After it's done, the master branch should be merged into #524 PR so that the diff there is much smaller and easier to review/finish it.

…ion. Ported changes from ckeditor/ckeditor5-utils#309 (1aac49db1ca9ea914f9fc44f00e295474faf3ed2). Note this breaks BodyCollection integration, so there's more work to be done.

…is is where it is added. Next commit will remove locale handling from the ViewCollection class.

…correctly." This reverts commit 8843250.

mlewand · 2020-02-27T10:08:53Z

Not sure what's up with the code coverage as I'm having 100% CC both, when running UI module alone and all the tests (here technically it's 99.9% due to ckeditor/ckeditor5#6326):

UI module:

Chrome 80.0.3987 (Windows 10.0.0): Executed 868 of 868 SUCCESS (10.824 secs / 1.831 secs)

=============================== Coverage summary ===============================
Statements   : 100% ( 1575/1575 )
Branches     : 100% ( 604/604 )
Functions    : 100% ( 450/450 )
Lines        : 100% ( 1549/1549 )
================================================================================

all:

Chrome 80.0.3987 (Windows 10.0.0): Executed 13317 of 13339 (skipped 22) SUCCESS (6 mins 4.766 secs / 2 mins 5.142 secs)

=============================== Coverage summary ===============================
Statements   : 100% ( 18726/18726 )
Branches     : 99.99% ( 9621/9622 )
Functions    : 100% ( 4190/4190 )
Lines        : 100% ( 18447/18447 )
================================================================================

coveralls · 2020-02-27T10:10:04Z

Coverage remained the same at 100.0% when pulling bd93bd0 on i/6319 into b30bdd9 on master.

oleq · 2020-03-25T13:17:34Z

src/view.js

 	 * @returns {module:ui/viewcollection~ViewCollection} A new collection of view instances.
 	 */
-	createCollection() {
-		const collection = new ViewCollection();
+	createCollection( views ) {


Missing docs for the usage with views.

Addressed in e470ac5.

oleq · 2020-03-25T13:19:47Z

src/viewcollection.js

-		 *
-		 * @member {module:utils/locale~Locale}
-		 */
-		this.locale = locale;


Was it really obsolete? Because it does not feel like the scope of the PR. OTOH it makes the arguments discovery harder in the constructor(), that's why I'm asking.

As you pointed out back in #524 - locale in ViewCollection has no usage in our codebase. However sine then BodyColleciton subclass has been introduced which does use it.

More on that in the PR main description:

Back then we could have removed locale from all collections. Today locale is required by one Collection subclass - BodyCollection (it uses it to determine text direction). So it is only BodyCollection that contains this property.

Because it does not feel like the scope of the PR

I do agree on the fact that it goes outside of the scope, I'm fine with extracting this - let me know if clarification are good enough or you'd prefer to extract it to a separate issue.

oleq · 2020-03-25T13:21:41Z

src/viewcollection.js

+			view.render();
+		}
+
+		if ( view.element && this._parentElement ) {


How can a View have no #element if it's been just rendered in :204?

That's unmodified original code from the master branch, it has just been extracted to a named method - and it wasn't really my intention to tackle issues not related with the issue.

Giving it a quick look the element could be unavailable if the view has no template sounds like an edge case, bot something that might happen? Eventually this refactoring might be extracted to a follow up in order not to block this PR.

oleq · 2020-03-25T13:22:53Z

src/editorui/bodycollection.js

+	 * @param {Iterable.<module:ui/view~View>} [initialItems] The initial items of the collection.
+	 */
+	constructor( locale, initialItems = [] ) {
+		super( initialItems );


I see no test that verifies that initialItems is passed to the ViewCollection#constructor.

Addressed in c71c323

oleq · 2020-03-27T10:40:14Z

Also, we should review all places that do createCollection or new ViewCollection and use the new API if possible. This may require some understanding of what's going on there, though.

…roperly handling preinitialized collection items.

mlewand · 2020-04-03T11:21:40Z

Also, we should review all places that do createCollection or new ViewCollection and use the new API if possible. This may require some understanding of what's going on there, though.

There will be quite a few of these and indeed it will involve checking surrounding code, so I'd like to do that once there are no other remaining points for this PR.

mlewand added 10 commits February 19, 2020 10:09

Feature: View collection can now also be initialized though construct…

bfe8744

…ion. Ported changes from ckeditor/ckeditor5-utils#309 (1aac49db1ca9ea914f9fc44f00e295474faf3ed2). Note this breaks BodyCollection integration, so there's more work to be done.

Internal: Restored locale support for BodyCollection to work correctly.

8843250

Internal: Locale instance is required only for BodyCollection - so th…

85da301

…is is where it is added. Next commit will remove locale handling from the ViewCollection class.

Revert "Internal: Restored locale support for BodyCollection to work …

d1e4ab7

…correctly." This reverts commit 8843250.

Tests: Added unit test coverage for BodyCollection constructor.

bcf5b4f

Internal: Collection now works on iterables.

eafba4e

Merge branch 'master' into i/6194

0e34bef

Merge branch 'master' into i/6194

8838e08

Docs: Views can be actually an iterable.

db27824

Merge branch 'master' into i/6194

ab499c1

mlewand mentioned this pull request Feb 27, 2020

Added support for initializing ViewCollection and BodyCollection items using a constructor #544

Closed

mlewand mentioned this pull request Feb 27, 2020

Reduced the initial table insertion dropdown render time ckeditor/ckeditor5-table#255

Merged

mlewand mentioned this pull request Mar 11, 2020

Added support for initializing Collection using a constructor ckeditor/ckeditor5-utils#326

Merged

Merge remote-tracking branch 'origin/master' into i/6319

de36478

oleq self-assigned this Mar 25, 2020

Merge branch 'master' into i/6319

016c666

oleq suggested changes Mar 25, 2020

View reviewed changes

mlewand added 3 commits April 3, 2020 10:57

Merge branch 'master' into i/6319

b20a754

Docs: Adjusted code snippet for the view#createCollection() method.

e470ac5

Tests: Added a test ensuring that the BodyCollection constructor is p…

c71c323

…roperly handling preinitialized collection items.

mlewand requested a review from oleq April 3, 2020 11:21

Merge branch 'master' into i/6319

bd93bd0

oleq merged commit 6cd15de into master Apr 6, 2020

oleq deleted the i/6319 branch April 6, 2020 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for initializing ViewCollection and BodyCollection items using a constructor #545

Added support for initializing ViewCollection and BodyCollection items using a constructor #545

mlewand commented Feb 27, 2020

mlewand commented Feb 27, 2020 •

edited

Loading

coveralls commented Feb 27, 2020 •

edited

Loading

oleq Mar 25, 2020

mlewand Apr 3, 2020

oleq Mar 25, 2020

mlewand Apr 3, 2020 •

edited

Loading

oleq Mar 25, 2020

mlewand Apr 3, 2020

oleq Mar 25, 2020

mlewand Apr 3, 2020

oleq commented Mar 27, 2020

mlewand commented Apr 3, 2020

Added support for initializing ViewCollection and BodyCollection items using a constructor #545

Added support for initializing ViewCollection and BodyCollection items using a constructor #545

Conversation

mlewand commented Feb 27, 2020

Suggested merge commit message (convention)

Additional information

mlewand commented Feb 27, 2020 • edited Loading

coveralls commented Feb 27, 2020 • edited Loading

oleq Mar 25, 2020

Choose a reason for hiding this comment

mlewand Apr 3, 2020

Choose a reason for hiding this comment

oleq Mar 25, 2020

Choose a reason for hiding this comment

mlewand Apr 3, 2020 • edited Loading

Choose a reason for hiding this comment

oleq Mar 25, 2020

Choose a reason for hiding this comment

mlewand Apr 3, 2020

Choose a reason for hiding this comment

oleq Mar 25, 2020

Choose a reason for hiding this comment

mlewand Apr 3, 2020

Choose a reason for hiding this comment

oleq commented Mar 27, 2020

mlewand commented Apr 3, 2020

mlewand commented Feb 27, 2020 •

edited

Loading

coveralls commented Feb 27, 2020 •

edited

Loading

mlewand Apr 3, 2020 •

edited

Loading