Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Added support for initializing Collection using a constructor #326

Merged
merged 14 commits into from
Mar 23, 2020
Merged

Conversation

mlewand
Copy link
Contributor

@mlewand mlewand commented Feb 26, 2020

Suggested merge commit message (convention)

Feature: Added support for initializing Collection items using a constructor. Closes ckeditor/ckeditor5#6319.


Additional information

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

  • Reverted collection-add-invalid-id and collection-add-item-already-exists error interfaces back to what we have on the master branch (see current vs originally proposed). I don't think there's a strong reason to change this API.
  • add() remains unchanged with exception of reusing _getItemIdBeforeAdding().
  • Added API docs.
  • Added couple more test cases: generates id for items that doesn\'t have it, throws an error when invalid item key is provided, throws an error when items have a duplicated key.

I had some doubts:

Once this PR is merged, the master branch should get merged to the #309 PR so that the diff will get smaller and easier to review.

There's a subpr: ckeditor/ckeditor5-ui#545

@coveralls
Copy link

coveralls commented Feb 26, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 32866ae on i/6319 into b148e9c on master.

@mlewand mlewand marked this pull request as ready for review February 27, 2020 08:24
@mlewand mlewand requested a review from oleq February 27, 2020 08:25
src/collection.js Outdated Show resolved Hide resolved
tests/collection.js Outdated Show resolved Hide resolved
tests/collection.js Outdated Show resolved Hide resolved
tests/collection.js Outdated Show resolved Hide resolved
tests/collection.js Outdated Show resolved Hide resolved
tests/collection.js Outdated Show resolved Hide resolved
tests/collection.js Outdated Show resolved Hide resolved
tests/collection.js Outdated Show resolved Hide resolved
@mlewand mlewand requested a review from oleq March 11, 2020 13:56
@oleq
Copy link
Member

oleq commented Mar 12, 2020

Let's merge it ASAP in the upcoming iteration.

@oleq oleq merged commit 8846e66 into master Mar 23, 2020
@oleq oleq deleted the i/6319 branch March 23, 2020 10:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collection constructor should allow to set initial values
4 participants