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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
bfe8744
Feature: View collection can now also be initialized though construct…
mlewand Feb 19, 2020
8843250
Internal: Restored locale support for BodyCollection to work correctly.
mlewand Feb 19, 2020
85da301
Internal: Locale instance is required only for BodyCollection - so th…
mlewand Feb 19, 2020
d1e4ab7
Revert "Internal: Restored locale support for BodyCollection to work …
mlewand Feb 19, 2020
bcf5b4f
Tests: Added unit test coverage for BodyCollection constructor.
mlewand Feb 19, 2020
eafba4e
Internal: Collection now works on iterables.
mlewand Feb 19, 2020
0e34bef
Merge branch 'master' into i/6194
mlewand Feb 24, 2020
8838e08
Merge branch 'master' into i/6194
mlewand Feb 26, 2020
db27824
Docs: Views can be actually an iterable.
mlewand Feb 26, 2020
ab499c1
Merge branch 'master' into i/6194
mlewand Feb 27, 2020
de36478
Merge remote-tracking branch 'origin/master' into i/6319
mlewand Mar 11, 2020
016c666
Merge branch 'master' into i/6319
oleq Mar 25, 2020
b20a754
Merge branch 'master' into i/6319
mlewand Apr 3, 2020
e470ac5
Docs: Adjusted code snippet for the view#createCollection() method.
mlewand Apr 3, 2020
c71c323
Tests: Added a test ensuring that the BodyCollection constructor is p…
mlewand Apr 3, 2020
bd93bd0
Merge branch 'master' into i/6319
oleq Apr 6, 2020
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
View file
Open in desktop
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
16 changes: 6 additions & 10 deletions src/view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,8 @@ export default class View {
* constructor( locale ) {
* super( locale );
*
* this.items = this.createCollection();
* const child = new ChildView( locale );
* this.items = this.createCollection( [ child ] );
*
* this.setTemplate( {
* tag: 'p',
Expand All @@ -265,21 +266,16 @@ export default class View {
* }
*
* const view = new SampleView( locale );
* const child = new ChildView( locale );
*
* view.render();
*
* // It will append <p></p> to the <body>.
* // It will append <p><child#element></p> to the <body>.
* document.body.appendChild( view.element );
*
* // From now on the child is nested under its parent, which is also reflected in DOM.
* // <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
View file
Open in desktop
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 •
edited
Loading

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
17 changes: 17 additions & 0 deletions tests/editorui/bodycollection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,23 @@ describe( 'BodyCollection', () => {
}
} );

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

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

it( 'stores pre-initialized collection', () => {
const collectionItems = [ new View(), new View() ];
const instance = new BodyCollection( locale, collectionItems );

expect( instance ).to.have.lengthOf( 2 );
expect( instance.get( 0 ) ).to.be.equal( collectionItems[ 0 ] );
expect( instance.get( 1 ) ).to.be.equal( collectionItems[ 1 ] );
} );
} );

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
View file
Open in desktop
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
View file
Open in desktop
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
Loading