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

Introduced the Collection.addMany method and Collection.change event #7644

Merged
merged 11 commits into from
Jul 20, 2020

Conversation

mlewand
Copy link
Contributor

@mlewand mlewand commented Jul 17, 2020

Suggested merge commit message (convention)

Feature: Introduced the Collection.addMany() method for adding multiple items in a single call. Closes #7627.

Feature: Introduced the Collection.change event. See #7627.


Additional information

With this I was also able to simplify DocumentColorCollection by using new change event.

clear() method uses almost the entire remove() logic except that it must not fire change for each removal. Thus I extracted a private base of remove() that can be reused in both cases.

@mlewand mlewand changed the title ### Suggested merge commit message ([convention](https://ckeditor.com/docs/ckeditor5/latest/framework/guides/contributing/git-commit-message-convention.html)) Introduced Collection.addMany method and Collection.change event Jul 17, 2020
@mlewand mlewand changed the title Introduced Collection.addMany method and Collection.change event Introduced the Collection.addMany method and Collection.change event Jul 17, 2020
Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation is OK (only one left over found) 👍

However I'd duplicate tests for add() and addMany() to document the feature we want not to test the implementation we have now.

packages/ckeditor5-utils/src/collection.js Outdated Show resolved Hide resolved
packages/ckeditor5-utils/tests/collection.js Outdated Show resolved Hide resolved
packages/ckeditor5-utils/tests/collection.js Outdated Show resolved Hide resolved
packages/ckeditor5-utils/tests/collection.js Show resolved Hide resolved
@mlewand mlewand requested a review from jodator July 20, 2020 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a method for adding multiple items to the collection at once
2 participants