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

i/4992: Used the new Collection and View API to add multiple Views at a time #56

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 25 additions & 16 deletions src/documentcolorcollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix';
* @extends module:utils/collection~Collection
*/
export default class DocumentColorCollection extends Collection {
constructor( options ) {
super( options );
constructor( ...args ) {
super( ...args );

/**
* Indicates whether the document color collection is empty.
Expand All @@ -23,32 +23,43 @@ export default class DocumentColorCollection extends Collection {
* @readonly
* @member {Boolean} #isEmpty
*/
this.set( 'isEmpty', true );
this.set( 'isEmpty', !this.length );
}

/**
* Adds a color to the document color collection.
* Adds a color (or colors) to the document color collection.
*
* This method ensures that no color duplicates are inserted (compared using
* the color value of the {@link module:ui/colorgrid/colorgrid~ColorDefinition}).
*
* If the item does not have an ID, it will be automatically generated and set on the item.
*
* @chainable
* @param {module:ui/colorgrid/colorgrid~ColorDefinition} item
* @param {Number} [index] The position of the item in the collection. The item
* is pushed to the collection when `index` is not specified.
* @param {...(module:ui/colorgrid/colorgrid~ColorDefinition)} items
Copy link
Contributor

Choose a reason for hiding this comment

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

I was looking at a similar construct in the uitls PR and AFAIC the () is not needed for at least primitives. Maybe in this case we could also drop them?

* @param {Number} [index] The position of the color (or colors) in the collection. Colors
* are pushed to the collection when `index` is not specified.
* @fires add
*/
add( item, index ) {
if ( this.find( element => element.color === item.color ) ) {
// No duplicates are allowed.
return;
add( ...args ) {
let items = args;

// No duplicates are allowed.
items = items.filter( ( item, index ) => {
// Don't filter out the add index (last argument).
if ( index === items.length - 1 && typeof item === 'number' ) {
return true;
}

return !this.find( element => element.color === item.color );
} );

if ( items.length ) {
super.add( ...items );
}

super.add( item, index );
this.set( 'isEmpty', !this.length );

this.set( 'isEmpty', false );
return this;
}

/**
Expand All @@ -57,9 +68,7 @@ export default class DocumentColorCollection extends Collection {
remove( subject ) {
const ret = super.remove( subject );

if ( this.length === 0 ) {
this.set( 'isEmpty', true );
}
this.set( 'isEmpty', !this.length );

return ret;
}
Expand Down
7 changes: 4 additions & 3 deletions src/fontfamily/fontfamilyui.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ export default class FontFamilyUI extends Plugin {
// @param {Array.<module:font/fontsize~FontSizeOption>} options
// @param {module:font/fontsize/fontsizecommand~FontSizeCommand} command
function _prepareListOptions( options, command ) {
const itemDefinitions = new Collection();
const itemDefinitions = [];

// Create dropdown items.
for ( const option of options ) {
Expand All @@ -116,7 +116,8 @@ function _prepareListOptions( options, command ) {
def.model.set( 'labelStyle', `font-family: ${ option.view.styles[ 'font-family' ] }` );
}

itemDefinitions.add( def );
itemDefinitions.push( def );
}
return itemDefinitions;

return new Collection( itemDefinitions );
}
6 changes: 3 additions & 3 deletions src/fontsize/fontsizeui.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export default class FontSizeUI extends Plugin {
// @param {Array.<module:font/fontsize~FontSizeOption>} options
// @param {module:font/fontsize/fontsizecommand~FontSizeCommand} command
function _prepareListOptions( options, command ) {
const itemDefinitions = new Collection();
const itemDefinitions = [];

for ( const option of options ) {
const def = {
Expand All @@ -135,8 +135,8 @@ function _prepareListOptions( options, command ) {
def.model.bind( 'isOn' ).to( command, 'value', value => value === option.model );

// Add the option to the collection.
itemDefinitions.add( def );
itemDefinitions.push( def );
}

return itemDefinitions;
return new Collection( itemDefinitions );
}
7 changes: 2 additions & 5 deletions src/ui/colortableview.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,7 @@ export default class ColorTableView extends View {
children: this.items
} );

this.items.add( this._removeColorButton() );
this.items.add( this.staticColorsGrid );
this.items.add( this._removeColorButton(), this.staticColorsGrid );

if ( documentColorsCount ) {
// Create a label for document colors.
Expand All @@ -182,10 +181,8 @@ export default class ColorTableView extends View {
}
} );

this.items.add( label );

this.documentColorsGrid = this._createDocumentColorsGrid();
this.items.add( this.documentColorsGrid );
this.items.add( label, this.documentColorsGrid );
}
}

Expand Down
19 changes: 14 additions & 5 deletions tests/documentcolorcollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,7 @@ describe( 'DocumentColorCollection', () => {
];

beforeEach( () => {
documentColorCollection = new DocumentColorCollection();

colors.forEach( item => {
documentColorCollection.add( item );
} );
documentColorCollection = new DocumentColorCollection( colors );
} );

it( 'constructor()', () => {
Expand Down Expand Up @@ -58,4 +54,17 @@ describe( 'DocumentColorCollection', () => {
expect( documentColorCollection.hasColor( '111' ) ).to.be.true;
expect( documentColorCollection.hasColor( '555' ) ).to.be.false;
} );

describe( 'add()', () => {
it( 'supports adding multiple items at a time', () => {
documentColorCollection = new DocumentColorCollection();

documentColorCollection.add( colors[ 0 ] );
documentColorCollection.add( colors[ 1 ], colors[ 2 ], 0 );

expect( documentColorCollection.map( item => item.color ) ).to.have.ordered.members( [
'222', '333', '111'
] );
} );
} );
} );