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

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

Merged
merged 16 commits into from
Apr 6, 2020
Merged
Show file tree
Hide file tree
Changes from 12 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
18 changes: 18 additions & 0 deletions src/editorui/bodycollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,24 @@ import createElement from '@ckeditor/ckeditor5-utils/src/dom/createelement';
* @extends module:ui/viewcollection~ViewCollection
*/
export default class BodyCollection extends ViewCollection {
/**
* Creates a new instance of the {@link module:ui/editorui/bodycollection~BodyCollection}.
*
* @param {module:utils/locale~Locale} locale The {@link module:core/editor/editor~Editor editor's locale} instance.
* @param {Iterable.<module:ui/view~View>} [initialItems] The initial items of the collection.
*/
constructor( locale, initialItems = [] ) {
super( initialItems );
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in c71c323


/**
* The {@link module:core/editor/editor~Editor#locale editor's locale} instance.
* See the view {@link module:ui/view~View#locale locale} property.
*
* @member {module:utils/locale~Locale}
*/
this.locale = locale;
}

/**
* Attaches the body collection to the DOM body element. You need to execute this method to render the content of
* the body collection.
Expand Down
5 changes: 3 additions & 2 deletions src/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -276,10 +276,11 @@ export default class View {
* // <p><child#element></p>
* view.items.add( child );
*
* @param {Iterable.<module:ui/view~View>} [views] Initial views of the collection.
* @returns {module:ui/viewcollection~ViewCollection} A new collection of view instances.
*/
createCollection() {
const collection = new ViewCollection();
createCollection( views ) {
Copy link
Member

Choose a reason for hiding this comment

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

Missing docs for the usage with views.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in e470ac5.

const collection = new ViewCollection( views );

this._viewCollections.add( collection );

Expand Down
51 changes: 33 additions & 18 deletions src/viewcollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,24 +51,18 @@ export default class ViewCollection extends Collection {
/**
* Creates a new instance of the {@link module:ui/viewcollection~ViewCollection}.
*
* @param {module:utils/locale~Locale} [locale] The {@link module:core/editor/editor~Editor editor's locale} instance.
* @param {Iterable.<module:ui/view~View>} [initialItems] The initial items of the collection.
*/
constructor( locale ) {
super( {
constructor( initialItems = [] ) {
super( initialItems, {
// An #id Number attribute should be legal and not break the `ViewCollection` instance.
// https://github.com/ckeditor/ckeditor5-ui/issues/93
idProperty: 'viewUid'
} );

// Handle {@link module:ui/view~View#element} in DOM when a new view is added to the collection.
this.on( 'add', ( evt, view, index ) => {
if ( !view.isRendered ) {
view.render();
}

if ( view.element && this._parentElement ) {
this._parentElement.insertBefore( view.element, this._parentElement.children[ index ] );
}
this._renderViewIntoCollectionParent( view, index );
} );

// Handle {@link module:ui/view~View#element} in DOM when a view is removed from the collection.
Expand All @@ -78,14 +72,6 @@ export default class ViewCollection extends Collection {
}
} );

/**
* The {@link module:core/editor/editor~Editor#locale editor's locale} instance.
* See the view {@link module:ui/view~View#locale locale} property.
*
* @member {module:utils/locale~Locale}
*/
this.locale = locale;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@mlewand mlewand Apr 3, 2020

Choose a reason for hiding this comment

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

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.


/**
* A parent element within which child views are rendered and managed in DOM.
*
Expand All @@ -112,6 +98,11 @@ export default class ViewCollection extends Collection {
*/
setParent( elementOrDocFragment ) {
this._parentElement = elementOrDocFragment;

// Take care of the initial collection items passed to the constructor.
for ( const view of this ) {
this._renderViewIntoCollectionParent( view );
}
}

/**
Expand Down Expand Up @@ -194,6 +185,30 @@ export default class ViewCollection extends Collection {
};
}

/**
* This method {@link module:ui/view~View#render renders} a new view added to the collection.
*
* If the {@link #_parentElement parent element} of the collection is set, this method also adds
* the view's {@link module:ui/view~View#element} as a child of the parent in DOM at a specified index.
*
* **Note**: If index is not specified, the view's element is pushed as the last child
* of the parent element.
*
* @private
* @param {module:ui/view~View} view A new view added to the collection.
* @param {Number} [index] An index the view holds in the collection. When not specified,
* the view is added at the end.
*/
_renderViewIntoCollectionParent( view, index ) {
if ( !view.isRendered ) {
view.render();
}

if ( view.element && this._parentElement ) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

this._parentElement.insertBefore( view.element, this._parentElement.children[ index ] );
}
}

/**
* Removes a child view from the collection. If the {@link #setParent parent element} of the
* collection has been set, the {@link module:ui/view~View#element element} of the view is also removed
Expand Down
8 changes: 8 additions & 0 deletions tests/editorui/bodycollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ describe( 'BodyCollection', () => {
}
} );

describe( 'constructor', () => {
it( 'assigns locale', () => {
const instance = new BodyCollection( locale );

expect( instance.locale ).to.be.equal( locale );
} );
} );

describe( 'attachToDom', () => {
it( 'should create wrapper and put the collection in that wrapper', () => {
const body = new BodyCollection( locale );
Expand Down
75 changes: 19 additions & 56 deletions tests/focuscycler.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,22 +17,21 @@ describe( 'FocusCycler', () => {
testUtils.createSinonSandbox();

beforeEach( () => {
focusables = new ViewCollection();
testUtils.sinon.stub( global.window, 'getComputedStyle' );
focusables = new ViewCollection( [
nonFocusable(),
focusable(),
focusable(),
focusable(),
nonFocusable()
] );
focusTracker = {
focusedElement: null
};
cycler = new FocusCycler( {
focusables,
focusTracker
} );

testUtils.sinon.stub( global.window, 'getComputedStyle' );

focusables.add( nonFocusable() );
focusables.add( focusable() );
focusables.add( focusable() );
focusables.add( focusable() );
focusables.add( nonFocusable() );
} );

describe( 'constructor()', () => {
Expand Down Expand Up @@ -60,12 +59,9 @@ describe( 'FocusCycler', () => {
} );

it( 'returns null when no focusable items', () => {
focusables = new ViewCollection();
focusables = new ViewCollection( [ nonFocusable(), nonFocusable() ] );
cycler = new FocusCycler( { focusables, focusTracker } );

focusables.add( nonFocusable() );
focusables.add( nonFocusable() );

expect( cycler.first ).to.be.null;
} );

Expand All @@ -83,12 +79,9 @@ describe( 'FocusCycler', () => {
} );

it( 'returns null when no focusable items', () => {
focusables = new ViewCollection();
focusables = new ViewCollection( [ nonFocusable(), nonFocusable() ] );
cycler = new FocusCycler( { focusables, focusTracker } );

focusables.add( nonFocusable() );
focusables.add( nonFocusable() );

expect( cycler.last ).to.be.null;
} );

Expand Down Expand Up @@ -126,23 +119,16 @@ describe( 'FocusCycler', () => {
} );

it( 'returns null when no focusable items', () => {
focusables = new ViewCollection();
focusables = new ViewCollection( [ nonFocusable(), nonFocusable() ] );
cycler = new FocusCycler( { focusables, focusTracker } );

focusables.add( nonFocusable() );
focusables.add( nonFocusable() );

expect( cycler.next ).to.be.null;
} );

it( 'returns null if the only focusable in focusables', () => {
focusables = new ViewCollection();
focusables = new ViewCollection( [ nonFocusable(), focusable(), nonFocusable() ] );
cycler = new FocusCycler( { focusables, focusTracker } );

focusables.add( nonFocusable() );
focusables.add( focusable() );
focusables.add( nonFocusable() );

focusTracker.focusedElement = focusables.get( 1 ).element;

expect( cycler.first ).to.equal( focusables.get( 1 ) );
Expand Down Expand Up @@ -176,23 +162,16 @@ describe( 'FocusCycler', () => {
} );

it( 'returns null when no focusable items', () => {
focusables = new ViewCollection();
focusables = new ViewCollection( [ nonFocusable(), nonFocusable() ] );
cycler = new FocusCycler( { focusables, focusTracker } );

focusables.add( nonFocusable() );
focusables.add( nonFocusable() );

expect( cycler.previous ).to.be.null;
} );

it( 'returns null if the only focusable in focusables', () => {
focusables = new ViewCollection();
focusables = new ViewCollection( [ nonFocusable(), focusable(), nonFocusable() ] );
cycler = new FocusCycler( { focusables, focusTracker } );

focusables.add( nonFocusable() );
focusables.add( focusable() );
focusables.add( nonFocusable() );

focusTracker.focusedElement = focusables.get( 1 ).element;

expect( cycler.first ).to.equal( focusables.get( 1 ) );
Expand All @@ -208,12 +187,9 @@ describe( 'FocusCycler', () => {
} );

it( 'does not throw when no focusable items', () => {
focusables = new ViewCollection();
focusables = new ViewCollection( [ nonFocusable(), nonFocusable() ] );
cycler = new FocusCycler( { focusables, focusTracker } );

focusables.add( nonFocusable() );
focusables.add( nonFocusable() );

expect( () => {
cycler.focusFirst();
} ).to.not.throw();
Expand All @@ -231,11 +207,7 @@ describe( 'FocusCycler', () => {
it( 'ignores invisible items', () => {
const item = focusable();

focusables = new ViewCollection();
focusables.add( nonFocusable() );
focusables.add( focusable( true ) );
focusables.add( item );

focusables = new ViewCollection( [ nonFocusable(), focusable( true ), item ] );
cycler = new FocusCycler( { focusables, focusTracker } );

cycler.focusFirst();
Expand All @@ -251,12 +223,9 @@ describe( 'FocusCycler', () => {
} );

it( 'does not throw when no focusable items', () => {
focusables = new ViewCollection();
focusables = new ViewCollection( [ nonFocusable(), nonFocusable() ] );
cycler = new FocusCycler( { focusables, focusTracker } );

focusables.add( nonFocusable() );
focusables.add( nonFocusable() );

expect( () => {
cycler.focusLast();
} ).to.not.throw();
Expand All @@ -281,12 +250,9 @@ describe( 'FocusCycler', () => {
} );

it( 'does not throw when no focusable items', () => {
focusables = new ViewCollection();
focusables = new ViewCollection( [ nonFocusable(), nonFocusable() ] );
cycler = new FocusCycler( { focusables, focusTracker } );

focusables.add( nonFocusable() );
focusables.add( nonFocusable() );

expect( () => {
cycler.focusNext();
} ).to.not.throw();
Expand All @@ -311,12 +277,9 @@ describe( 'FocusCycler', () => {
} );

it( 'does not throw when no focusable items', () => {
focusables = new ViewCollection();
focusables = new ViewCollection( [ nonFocusable(), nonFocusable() ] );
cycler = new FocusCycler( { focusables, focusTracker } );

focusables.add( nonFocusable() );
focusables.add( nonFocusable() );

expect( () => {
cycler.focusPrevious();
} ).to.not.throw();
Expand Down
11 changes: 11 additions & 0 deletions tests/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,17 @@ describe( 'View', () => {
expect( view._viewCollections ).to.have.length( 2 );
expect( view._viewCollections.get( 1 ) ).to.equal( collection );
} );

it( 'accepts initial views', () => {
const viewA = new View();
const viewB = new View();

const collection = view.createCollection( [ viewA, viewB ] );

expect( collection ).to.have.length( 2 );
expect( collection.get( 0 ) ).to.equal( viewA );
expect( collection.get( 1 ) ).to.equal( viewB );
} );
} );

describe( 'registerChild()', () => {
Expand Down
33 changes: 29 additions & 4 deletions tests/viewcollection.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,18 @@ describe( 'ViewCollection', () => {

describe( 'constructor()', () => {
it( 'sets basic properties and attributes', () => {
expect( collection.locale ).to.be.undefined;
expect( collection._parentElement ).to.be.null;
expect( collection._idProperty ).to.equal( 'viewUid' );
} );

it( 'accepts locale and defines the locale property', () => {
const locale = { t() {} };
it( 'allows setting initial collection items', () => {
const view1 = new View();
const view2 = new View();
const collection = new ViewCollection( [ view1, view2 ] );

expect( new ViewCollection( locale ).locale ).to.equal( locale );
expect( collection ).to.have.length( 2 );
expect( collection.get( 0 ) ).to.equal( view1 );
expect( collection.get( 1 ) ).to.equal( view2 );
} );

describe( 'child view management in DOM', () => {
Expand Down Expand Up @@ -165,6 +168,28 @@ describe( 'ViewCollection', () => {
collection.setParent( el );
expect( collection._parentElement ).to.equal( el );
} );

it( 'udpates initial collection items in DOM', () => {
const view1 = new View();
view1.element = document.createElement( 'i' );
sinon.spy( view1, 'render' );

const view2 = new View();
view2.element = document.createElement( 'b' );
sinon.spy( view2, 'render' );

const collection = new ViewCollection( [ view1, view2 ] );
const parentElement = document.createElement( 'div' );

expect( collection ).to.have.length( 2 );
expect( collection.get( 0 ) ).to.equal( view1 );
expect( collection.get( 1 ) ).to.equal( view2 );

collection.setParent( parentElement );
expect( normalizeHtml( parentElement.outerHTML ) ).to.equal( '<div><i></i><b></b></div>' );
sinon.assert.calledOnce( view1.render );
sinon.assert.calledOnce( view2.render );
} );
} );

describe( 'delegate()', () => {
Expand Down