From f7aad281d048bbec86ae44e0b43fc179808367cb Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 17 Oct 2017 17:17:29 +0200 Subject: [PATCH 01/44] Replaced View#element on-demand rendering with the #render method. Renamed addChildren(), removeChildren() -> registerChildren(), deregisterChildren(). --- src/view.js | 242 ++++++++++++++++++++++---------------------------- tests/view.js | 75 +++++++++------- 2 files changed, 148 insertions(+), 169 deletions(-) diff --git a/src/view.js b/src/view.js index 1df7c663..9112c243 100644 --- a/src/view.js +++ b/src/view.js @@ -21,9 +21,9 @@ import isIterable from '@ckeditor/ckeditor5-utils/src/isiterable'; * {@link module:ui/view~View#template}. Views are building blocks of the user interface and handle * interaction * - * Views {@link module:ui/view~View#addChildren aggregate} children in + * Views {@link module:ui/view~View#registerChildren aggregate} children in * {@link module:ui/view~View#createCollection collections} and manage the life cycle of DOM - * listeners e.g. by handling initialization and destruction. + * listeners e.g. by handling rendering and destruction. * * See the {@link module:ui/template~TemplateDefinition} syntax to learn more about shaping view * elements, attributes and listeners. @@ -66,8 +66,7 @@ import isIterable from '@ckeditor/ckeditor5-utils/src/isiterable'; * * const view = new SampleView( locale ); * - * // Each view must be first initialized. - * view.init(); + * view.render(); * * // Append

Helloworld

to the * document.body.appendChild( view.element ); @@ -86,11 +85,52 @@ export default class View { /** * Creates an instance of the {@link module:ui/view~View} class. * - * Also see {@link #init}. + * Also see {@link #render}. * * @param {module:utils/locale~Locale} [locale] The localization services instance. */ constructor( locale ) { + /** + * An HTML element of the view. `null` until {@link #render rendered} + * from the {@link #template}. + * + * class SampleView extends View { + * constructor() { + * super(); + * + * // A template instance the #element will be created from. + * this.template = new Template( { + * tag: 'p' + * + * // ... + * } ); + * } + * } + * + * const view = new SampleView(); + * + * // Renders the #template + * view.render(); + * + * // Append the HTML element of the view to . + * document.body.appendChild( view.element ); + * + * **Note**: The element of the view can also be assigned directly: + * + * view.element = document.querySelector( '#my-container' ); + * + * @member {HTMLElement} + */ + this.element = null; + + /** + * Set `true` when the view has already been {@link module:ui/view~View#render rendered}. + * + * @readonly + * @member {Boolean} #isRendered + */ + this.isRendered = false; + /** * A set of tools to localize the user interface. * @@ -112,17 +152,6 @@ export default class View { */ this.t = locale && locale.t; - /** - * A flag set `true` after {@link #init initialization}. Because the process can be - * asynchronous, this {@link module:utils/observablemixin~Observable observable} flag allows - * deferring certain actions until it is finished. - * - * @readonly - * @observable - * @member {Boolean} #ready - */ - this.set( 'ready', false ); - /** * Collections registered with {@link #createCollection}. * @@ -147,18 +176,11 @@ export default class View { /** * Template of this view. It provides the {@link #element} representing - * the view in DOM. + * the view in DOM, which is {@link #render rendered}. * * @member {module:ui/template~Template} #template */ - /** - * An internal, cached element of this view. See {@link #element}. - * - * @private - * @member {HTMLElement} #_element - */ - /** * Cached {@link @link module:ui/template~BindChain bind chain} object created by the * {@link #template}. See {@link #bindTemplate}. @@ -168,54 +190,6 @@ export default class View { */ } - /** - * An HTML element of this view. The element is rendered on first reference - * by the {@link #template}: - * - * class SampleView extends View { - * constructor() { - * super(); - * - * // A template instance the #element will be created from. - * this.template = new Template( { - * tag: 'p' - * - * // ... - * } ); - * } - * } - * - * const view = new SampleView(); - * view.init(); - * - * // Renders the #template and appends the output to . - * document.body.appendChild( view.element ); - * - * The element of the view can also be assigned directly: - * - * view.element = document.querySelector( '#my-container' ); - * - * @type {HTMLElement} - */ - get element() { - if ( this._element ) { - return this._element; - } - - // No template means no element (a virtual view). - if ( !this.template ) { - return null; - } - - this._addTemplateChildren(); - - return ( this._element = this.template.render() ); - } - - set element( el ) { - this._element = el; - } - /** * Shorthand for {@link module:ui/template~Template.bind}, a binding * {@link module:ui/template~BindChain interface} pre–configured for the view instance. @@ -289,7 +263,7 @@ export default class View { * const view = new SampleView( locale ); * const child = new ChildView( locale ); * - * view.init(); + * view.render(); * * // It will append

to the . * document.body.appendChild( view.element ); @@ -310,7 +284,7 @@ export default class View { /** * Registers a new child view under the view instance. Once registered, a child - * view is managed by its parent, including {@link #init initization} + * view is managed by its parent, including {@link #render rendering} * and {@link #destroy destruction}. * * To revert this, use {@link #removeChildren}. @@ -325,20 +299,20 @@ export default class View { * this.template = new Template( { tag: 'p' } ); * * // Register the children. - * this.addChildren( [ this.childA, this.childB ] ); + * this.registerChildren( [ this.childA, this.childB ] ); * } * - * init() { + * render() { + * super.render(); + * * this.element.appendChild( this.childA.element ); * this.element.appendChild( this.childB.element ); - * - * return super.init(); * } * } * * const view = new SampleView( locale ); * - * view.init(); + * view.render(); * * // Will append

. * document.body.appendChild( view.element ); @@ -357,7 +331,7 @@ export default class View { * tag: 'p', * * // These children will be added automatically. There's no - * // need to call {@link #addChildren} for any of them. + * // need to call {@link #registerChildren} for any of them. * children: [ this.childA, this.childB ] * } ); * } @@ -367,39 +341,53 @@ export default class View { * * @param {module:ui/view~View|Iterable.} children Children views to be registered. */ - addChildren( children ) { + registerChildren( children ) { if ( !isIterable( children ) ) { children = [ children ]; } - children.map( c => this._unboundChildren.add( c ) ); + for ( const child of children ) { + this._unboundChildren.add( child ); + } } /** - * The opposite of {@link #addChildren}. Removes a child view from this view instance. + * The opposite of {@link #registerChildren}. Removes a child view from this view instance. * Once removed, the child is no longer managed by its parent, e.g. it can safely * become a child of another parent view. * - * @see #addChildren + * @see #registerChildren * @param {module:ui/view~View|Iterable.} children Child views to be removed. */ - removeChildren( children ) { + deregisterChildren( children ) { if ( !isIterable( children ) ) { children = [ children ]; } - children.map( c => this._unboundChildren.remove( c ) ); + for ( const child of children ) { + this._unboundChildren.remove( child ); + } } /** - * Initializes the view and its children added by {@link #addChildren} and residing in collections - * created by the {@link #createCollection} method. + * Recursively renders the view. + * + * Once the view is rendered: + * * the {@link #element} becomes an HTML element out of {@link #template}, + * * the {@link #isRendered} flag is set `true`. + * + * **Note**: The children of the view: + * * defined directly in the {@link #template} + * * residing in collections created by the {@link #createCollection} method, + * * and added by {@link #registerChildren} + * are also rendered in the process. + * + * In general, `render()` method is the right place to keep the code which refers to the + * {@link #element} and should be executed at the very beginning of the view's life cycle. * - * In general, `init()` is the right place to keep the code which refers to the {@link #element} - * and should be executed at the very beginning of the view's life cycle. It is possible to - * {@link module:ui/template~Template.extend} the {@link #template} before the first reference of - * the {@link #element}. To allow an early customization of the view (e.g. by its parent), - * such references should be done in `init()`. + * It is possible to {@link module:ui/template~Template.extend} the {@link #template} before + * the view is rendered. To allow an early customization of the view (e.g. by its parent), + * such references should be done in `render()`. * * class SampleView extends View { * constructor() { @@ -408,26 +396,25 @@ export default class View { * } ); * }, * - * init() { - * super.init(); + * render() { + * // View#element becomes available. + * super.render(); * - * function scroll() { + * // The "scroll" listener depends on #element. + * this.listenTo( window, 'scroll', () => { * // A reference to #element would render the #template and make it non-extendable. * if ( window.scrollY > 0 ) { * this.element.scrollLeft = 100; * } else { * this.element.scrollLeft = 0; * } - * } - * - * this.listenTo( window, 'scroll', () => { - * scroll(); * } ); * } * } * * const view = new SampleView(); * + * // Let's customize the view before it gets rendered. * Template.extend( view.template, { * attributes: { * class: [ @@ -436,30 +423,35 @@ export default class View { * } * } ); * - * // Late initialization allows customization of the view. - * view.init(); - * - * Once initialized, the view becomes {@link #ready}. + * // Late rendering allows customization of the view. + * view.render(); */ - init() { - if ( this.ready ) { + render() { + if ( this.isRendered ) { /** - * This View has already been initialized. + * This View has already been rendered. * - * @error ui-view-init-reinit + * @error ui-view-render-rendered */ - throw new CKEditorError( 'ui-view-init-reinit: This View has already been initialized.' ); + throw new CKEditorError( 'ui-view-render-already-rendered: This View has already been rendered.' ); } - // Initialize collections in #_viewCollections. - this._viewCollections.map( c => c.init() ); + // Render collections in #_viewCollections. + this._viewCollections.map( col => col.render() ); - // Spread the word that this view is ready! - this.ready = true; + // Render #element of the view. + if ( this.template ) { + this.element = this.template.render(); + + // Auto–register view children from #template. + this.registerChildren( this.template.getViews() ); + } + + this.isRendered = true; } /** - * Recursively destroys the view instance and child views added by {@link #addChildren} and + * Recursively destroys the view instance and child views added by {@link #registerChildren} and * residing in collections created by the {@link #createCollection}. * * Destruction disables all event listeners: @@ -471,28 +463,6 @@ export default class View { this._viewCollections.map( c => c.destroy() ); } - - /** - * Recursively traverses {@link #template} in search of {@link module:ui/view~View} - * instances and automatically registers them using {@link #addChildren} method. - * - * @protected - */ - _addTemplateChildren() { - const search = def => { - if ( def.children ) { - for ( const defOrView of def.children ) { - if ( defOrView instanceof View ) { - this.addChildren( defOrView ); - } else { - search( defOrView ); - } - } - } - }; - - search( this.template ); - } } mix( View, DomEmitterMixin ); diff --git a/tests/view.js b/tests/view.js index 71543861..b075d256 100644 --- a/tests/view.js +++ b/tests/view.js @@ -28,7 +28,7 @@ describe( 'View', () => { expect( view.t ).to.be.undefined; expect( view.locale ).to.be.undefined; - expect( view.ready ).to.be.false; + expect( view.isRendered ).to.be.false; expect( view.template ).to.be.undefined; expect( view._viewCollections ).to.be.instanceOf( Collection ); expect( view._unboundChildren ).to.be.instanceOf( ViewCollection ); @@ -81,7 +81,7 @@ describe( 'View', () => { } ); } ); - describe( 'addChildren()', () => { + describe( 'registerChildren()', () => { beforeEach( () => { setTestViewClass(); setTestViewInstance(); @@ -90,9 +90,9 @@ describe( 'View', () => { it( 'should add a single view to #_unboundChildren', () => { expect( view._unboundChildren ).to.have.length( 0 ); - const child = {}; + const child = new View(); - view.addChildren( child ); + view.registerChildren( child ); expect( view._unboundChildren ).to.have.length( 1 ); expect( view._unboundChildren.get( 0 ) ).to.equal( child ); } ); @@ -100,74 +100,82 @@ describe( 'View', () => { it( 'should support iterables', () => { expect( view._unboundChildren ).to.have.length( 0 ); - view.addChildren( [ {}, {}, {} ] ); + view.registerChildren( [ new View(), new View(), new View() ] ); expect( view._unboundChildren ).to.have.length( 3 ); } ); } ); - describe( 'removeChildren()', () => { + describe( 'deregisterChildren()', () => { beforeEach( () => { setTestViewClass(); setTestViewInstance(); } ); it( 'should remove a single view from #_unboundChildren', () => { - const child1 = {}; - const child2 = {}; + const child1 = new View(); + const child2 = new View(); - view.addChildren( child1 ); - view.addChildren( child2 ); + view.registerChildren( child1 ); + view.registerChildren( child2 ); expect( view._unboundChildren ).to.have.length( 2 ); - view.removeChildren( child2 ); + view.deregisterChildren( child2 ); expect( view._unboundChildren ).to.have.length( 1 ); expect( view._unboundChildren.get( 0 ) ).to.equal( child1 ); } ); it( 'should support iterables', () => { - const child1 = {}; - const child2 = {}; - const child3 = {}; + const child1 = new View(); + const child2 = new View(); + const child3 = new View(); - view.addChildren( [ child1, child2, child3 ] ); + view.registerChildren( [ child1, child2, child3 ] ); expect( view._unboundChildren ).to.have.length( 3 ); - view.removeChildren( [ child2, child3 ] ); + view.deregisterChildren( [ child2, child3 ] ); expect( view._unboundChildren ).to.have.length( 1 ); expect( view._unboundChildren.get( 0 ) ).to.equal( child1 ); } ); } ); - describe( 'init()', () => { - beforeEach( createViewWithChildren ); + describe( 'render()', () => { + it( 'should throw if already rendered', () => { + const view = new View(); - it( 'should throw if already initialized', () => { - view.init(); + view.render(); try { - view.init(); + view.render(); throw new Error( 'This should not be executed.' ); } catch ( err ) { expect( err ).to.be.instanceof( CKEditorError ); - expect( err.message ).to.match( /ui-view-init-re/ ); + expect( err.message ).to.match( /^ui-view-render-already-rendered:/ ); } } ); - it( 'should set view#ready', () => { - expect( view.ready ).to.be.false; + it( 'should set view#isRendered', () => { + const view = new View(); + + view.template = new Template( { + tag: 'div' + } ); + + expect( view.isRendered ).to.be.false; - view.init(); - expect( view.ready ).to.be.true; + view.render(); + expect( view.isRendered ).to.be.true; } ); - it( 'calls init() on all view#_viewCollections', () => { + it( 'calls render() on all view#_viewCollections', () => { + const view = new View(); + const collectionA = view.createCollection(); const collectionB = view.createCollection(); - const spyA = testUtils.sinon.spy( collectionA, 'init' ); - const spyB = testUtils.sinon.spy( collectionB, 'init' ); + const spyA = testUtils.sinon.spy( collectionA, 'render' ); + const spyB = testUtils.sinon.spy( collectionB, 'render' ); - view.init(); + view.render(); sinon.assert.calledOnce( spyA ); sinon.assert.calledOnce( spyB ); sinon.assert.callOrder( spyA, spyB ); @@ -248,8 +256,7 @@ describe( 'View', () => { expect( view._unboundChildren ).to.have.length( 0 ); - // Render the view. - view.element; + view.render(); expect( view._unboundChildren ).to.have.length( 3 ); expect( view._unboundChildren.get( 0 ) ).to.equal( viewA ); @@ -282,7 +289,7 @@ describe( 'View', () => { it( 'should not clear the #_unboundChildren', () => { const cached = view._unboundChildren; - view.addChildren( [ new View(), new View() ] ); + view.registerChildren( [ new View(), new View() ] ); expect( cached ).to.have.length( 4 ); view.destroy(); @@ -354,6 +361,8 @@ function setTestViewInstance() { view = new TestView(); if ( view.template ) { + view.render(); + document.body.appendChild( view.element ); } } From 40340174f48a8ac5c065451bec9bad8582b22034 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Tue, 17 Oct 2017 17:47:30 +0200 Subject: [PATCH 02/44] Aligned Template class to the View#render API. Changed type of Template#children (ViewCollection->Array). Implemented Template#getViews() generator. --- src/template.js | 63 +++++++++++++++++++++++++++++++++++++++++------ tests/template.js | 62 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 102 insertions(+), 23 deletions(-) diff --git a/src/template.js b/src/template.js index 2d3bfdcf..991baaed 100644 --- a/src/template.js +++ b/src/template.js @@ -103,7 +103,7 @@ export default class Template { * * **Note**: This property only makes sense when {@link #tag} is defined. * - * @member {module:utils/collection~Collection.} #children + * @member {Array.} #children */ /** @@ -209,6 +209,50 @@ export default class Template { this._revertTemplateFromNode( node, this._revertData ); } + /** + * Returns an iterator which traverses the template in search of {@link module:ui/view~View} + * instances and returns them one by one. + * + * const viewFoo = new View(); + * const viewBar = new View(); + * const viewBaz = new View(); + * const template = new Template( { + * tag: 'div', + * children: [ + * viewFoo, + * { + * tag: 'div', + * children: [ + * viewBar + * ] + * }, + * viewBaz + * ] + * } ); + * + * // Logs: viewFoo, viewBar, viewBaz + * for ( const view of template.getViews() ) { + * console.log( view ); + * } + * + * @returns {Iterator.} + */ + * getViews() { + function* search( def ) { + if ( def.children ) { + for ( const child of def.children ) { + if ( isView( child ) ) { + yield child; + } else { + yield* search( child ); + } + } + } + } + + yield* search( this ); + } + /** * An entry point to the interface which binds DOM nodes to * {@link module:utils/observablemixin~Observable observables}. @@ -305,7 +349,7 @@ export default class Template { * } ); * * // Child extension. - * Template.extend( template.children.get( 0 ), { + * Template.extend( template.children[ 0 ], { * attributes: { * class: 'd' * } @@ -643,6 +687,7 @@ export default class Template { if ( isViewCollection( child ) ) { if ( !isApplying ) { child.setParent( node ); + child.render(); for ( const view of child ) { container.appendChild( view.element ); @@ -650,6 +695,10 @@ export default class Template { } } else if ( isView( child ) ) { if ( !isApplying ) { + if ( !child.isRendered ) { + child.render(); + } + container.appendChild( child.element ); } } else { @@ -1117,17 +1166,17 @@ function normalize( def ) { normalizeAttributes( def.attributes ); } - const children = new ViewCollection(); + const children = []; if ( def.children ) { if ( isViewCollection( def.children ) ) { - children.add( def.children ); + children.push( def.children ); } else { for ( const child of def.children ) { if ( isTemplate( child ) || isView( child ) ) { - children.add( child ); + children.push( child ); } else { - children.add( new Template( child ) ); + children.push( new Template( child ) ); } } } @@ -1336,7 +1385,7 @@ function extendTemplate( template, def ) { let childIndex = 0; for ( const childDef of def.children ) { - extendTemplate( template.children.get( childIndex++ ), childDef ); + extendTemplate( template.children[ childIndex++ ], childDef ); } } } diff --git a/tests/template.js b/tests/template.js index 91a1b0ca..3c8ad60e 100644 --- a/tests/template.js +++ b/tests/template.js @@ -13,7 +13,6 @@ import Model from '../src/model'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin'; import DomEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin'; -import Collection from '@ckeditor/ckeditor5-utils/src/collection'; import normalizeHtml from '@ckeditor/ckeditor5-utils/tests/_utils/normalizehtml'; import log from '@ckeditor/ckeditor5-utils/src/log'; @@ -67,11 +66,11 @@ describe( 'Template', () => { expect( tpl.attributes.c[ 0 ].value[ 0 ] ).to.be.instanceof( TemplateToBinding ); expect( tpl.children ).to.have.length( 4 ); - expect( tpl.children.get( 0 ).text[ 0 ] ).to.equal( 'content' ); - expect( tpl.children.get( 1 ).text[ 0 ] ).to.be.instanceof( TemplateToBinding ); - expect( tpl.children.get( 2 ).text[ 0 ] ).to.equal( 'abc' ); - expect( tpl.children.get( 3 ).text[ 0 ] ).to.equal( 'a' ); - expect( tpl.children.get( 3 ).text[ 1 ] ).to.equal( 'b' ); + expect( tpl.children[ 0 ].text[ 0 ] ).to.equal( 'content' ); + expect( tpl.children[ 1 ].text[ 0 ] ).to.be.instanceof( TemplateToBinding ); + expect( tpl.children[ 2 ].text[ 0 ] ).to.equal( 'abc' ); + expect( tpl.children[ 3 ].text[ 0 ] ).to.equal( 'a' ); + expect( tpl.children[ 3 ].text[ 1 ] ).to.equal( 'b' ); expect( tpl.eventListeners[ 'a@span' ][ 0 ] ).to.be.instanceof( TemplateToBinding ); expect( tpl.eventListeners[ 'b@span' ][ 0 ] ).to.be.instanceof( TemplateToBinding ); @@ -92,8 +91,9 @@ describe( 'Template', () => { text: 'foo' } ); - expect( elementTpl.children ).to.be.instanceof( Collection ); + expect( elementTpl.children ).to.be.an( 'array' ); expect( elementTpl.children ).to.have.length( 0 ); + // Text will never have children. expect( textTpl.children ).to.be.undefined; } ); @@ -114,12 +114,12 @@ describe( 'Template', () => { expect( def.attributes ).to.not.equal( tpl.attributes ); expect( def.children ).to.not.equal( tpl.children ); - expect( def.children[ 0 ] ).to.not.equal( tpl.children.get( 0 ) ); + expect( def.children[ 0 ] ).to.not.equal( tpl.children[ 0 ] ); expect( def.attributes.a ).to.equal( 'foo' ); expect( def.children[ 0 ].tag ).to.equal( 'span' ); expect( tpl.attributes.a[ 0 ] ).to.equal( 'foo' ); - expect( tpl.children.get( 0 ).tag ).to.equal( 'span' ); + expect( tpl.children[ 0 ].tag ).to.equal( 'span' ); } ); } ); @@ -448,8 +448,8 @@ describe( 'Template', () => { children: [ v1, v2 ] } ); - expect( tpl.children.get( 0 ) ).to.equal( v1 ); - expect( tpl.children.get( 1 ) ).to.equal( v2 ); + expect( tpl.children[ 0 ] ).to.equal( v1 ); + expect( tpl.children[ 1 ] ).to.equal( v2 ); const rendered = tpl.render(); @@ -562,9 +562,9 @@ describe( 'Template', () => { } ); // Make sure child instances weren't cloned. - expect( tpl.children.get( 0 ) ).to.equal( childTplA ); - expect( tpl.children.get( 1 ) ).to.equal( childTplB ); - expect( tpl.children.get( 2 ) ).to.equal( childTplC ); + expect( tpl.children[ 0 ] ).to.equal( childTplA ); + expect( tpl.children[ 1 ] ).to.equal( childTplB ); + expect( tpl.children[ 2 ] ).to.equal( childTplC ); expect( normalizeHtml( tpl.render().outerHTML ) ).to.equal( '

foo

' @@ -1441,6 +1441,36 @@ describe( 'Template', () => { } ); } ); + describe( 'getViews()', () => { + it( 'returns iterator', () => { + const template = new Template( {} ); + + expect( template.getViews().next ).to.be.a( 'function' ); + expect( Array.from( template.getViews() ) ).to.have.length( 0 ); + } ); + + it( 'returns all child views', () => { + const viewA = new View(); + const viewB = new View(); + const viewC = new View(); + const template = new Template( { + tag: 'div', + children: [ + viewA, + { + tag: 'div', + children: [ + viewB + ] + }, + viewC + ] + } ); + + expect( Array.from( template.getViews() ) ).to.have.members( [ viewA, viewB, viewC ] ); + } ); + } ); + describe( 'bind()', () => { it( 'returns object', () => { expect( Template.bind() ).to.be.an( 'object' ); @@ -2739,7 +2769,7 @@ describe( 'Template', () => { ] } ); - Template.extend( template.children.get( 0 ), { + Template.extend( template.children[ 0 ], { attributes: { class: 'bar' } @@ -2773,7 +2803,7 @@ describe( 'Template', () => { ] } ); - Template.extend( template.children.get( 0 ), { + Template.extend( template.children[ 0 ], { attributes: { class: 'B', }, From 55cc8f99a29de6ea566a71356df20b5b77413253 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 18 Oct 2017 11:20:22 +0200 Subject: [PATCH 03/44] Aligned the ViewCollection class to the View#render API. Minor refactoring in View and Template classes. --- src/template.js | 2 +- src/view.js | 3 -- src/viewcollection.js | 79 +++++------------------------------------ tests/template.js | 24 ++++++++++--- tests/view.js | 15 -------- tests/viewcollection.js | 66 ++++------------------------------ 6 files changed, 34 insertions(+), 155 deletions(-) diff --git a/src/template.js b/src/template.js index 991baaed..364d8b1d 100644 --- a/src/template.js +++ b/src/template.js @@ -687,8 +687,8 @@ export default class Template { if ( isViewCollection( child ) ) { if ( !isApplying ) { child.setParent( node ); - child.render(); + // Note: ViewCollection renders its children. for ( const view of child ) { container.appendChild( view.element ); } diff --git a/src/view.js b/src/view.js index 9112c243..da7cfcba 100644 --- a/src/view.js +++ b/src/view.js @@ -436,9 +436,6 @@ export default class View { throw new CKEditorError( 'ui-view-render-already-rendered: This View has already been rendered.' ); } - // Render collections in #_viewCollections. - this._viewCollections.map( col => col.render() ); - // Render #element of the view. if ( this.template ) { this.element = this.template.render(); diff --git a/src/viewcollection.js b/src/viewcollection.js index 1ed7f6a2..9e3e5931 100644 --- a/src/viewcollection.js +++ b/src/viewcollection.js @@ -23,16 +23,13 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix'; * const viewA = new ChildView( locale ); * const viewB = new ChildView( locale ); * - * View collection manages view {@link module:ui/view~View#element elements}: + * View collection renders and manages view {@link module:ui/view~View#element elements}: * - * // parentView.element.children == [ viewA.element, vievB.element ] * collection.add( viewA ); * collection.add( viewB ); * - * It handles initialization of the children: - * - * // viewA.ready == viewB.ready == true; - * collection.init(); + * console.log( parentView.element.firsChild ); // -> viewA.element + * console.log( parentView.element.lastChild ); // -> viewB.element * * It {@link module:ui/viewcollection~ViewCollection#delegate propagates} DOM events too: * @@ -67,6 +64,10 @@ export default class ViewCollection extends Collection { // 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 ] ); } @@ -87,16 +88,6 @@ export default class ViewCollection extends Collection { */ this.locale = locale; - /** - * Set `true` when all views in the collection are {@link module:ui/view~View#ready}. - * See the view {@link module:ui/view~View#init init} method. - * - * @readonly - * @observable - * @member {Boolean} #ready - */ - this.set( 'ready', false ); - /** * A parent element within which child views are rendered and managed in DOM. * @@ -106,66 +97,12 @@ export default class ViewCollection extends Collection { this._parentElement = null; } - /** - * Initializes all child views in the collection by calling view {@link module:ui/view~View#init} - * method. - * - * Once finished, sets {@link #ready} `true`. - */ - init() { - if ( this.ready ) { - /** - * This ViewCollection has already been initialized. - * - * @error ui-viewcollection-init-reinit - */ - throw new CKEditorError( 'ui-viewcollection-init-reinit: This ViewCollection has already been initialized.' ); - } - - this.map( v => v.init() ); - - this.ready = true; - } - /** * Destroys the view collection along with child views. * See the view {@link module:ui/view~View#destroy} method. */ destroy() { - this.map( v => v.destroy() ); - } - - /** - * Adds a new child view to the collection. If the collection is - * {@link module:ui/viewcollection~ViewCollection#ready}, the child view is also - * {@link module:ui/view~View#init initialized} when added. - * - * Additionally, 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 added in DOM, - * reflecting the order of the collection. - * - * const collection = new ViewCollection(); - * const parentElement = document.querySelector( '#container' ); - * collection.setParent( parentElement ); - * - * const viewA = new View(); - * const viewB = new View(); - * - * // parentElement.children == [ viewA.element, vievB.element ] - * collection.add( viewA ); - * collection.add( viewB ); - * - * See the {@link #remove} method. - * - * @param {module:ui/view~View} view A child view. - * @param {Number} [index] Index at which the child will be added to the collection. - */ - add( view, index ) { - super.add( view, index ); - - if ( this.ready && !view.ready ) { - view.init(); - } + this.map( view => view.destroy() ); } /** diff --git a/tests/template.js b/tests/template.js index 3c8ad60e..e013e8a2 100644 --- a/tests/template.js +++ b/tests/template.js @@ -426,7 +426,7 @@ describe( 'Template', () => { it( 'renders view children', () => { const v1 = getView( { - tag: 'span', + tag: 'b', attributes: { class: [ 'v1' @@ -435,7 +435,7 @@ describe( 'Template', () => { } ); const v2 = getView( { - tag: 'span', + tag: 'b', attributes: { class: [ 'v2' @@ -443,22 +443,36 @@ describe( 'Template', () => { } } ); + const v3 = getView( { + tag: 'b', + attributes: { + class: [ + 'v3' + ] + } + } ); + + v3.render(); + const tpl = new Template( { tag: 'p', - children: [ v1, v2 ] + children: [ v1, v2, v3 ] } ); expect( tpl.children[ 0 ] ).to.equal( v1 ); expect( tpl.children[ 1 ] ).to.equal( v2 ); + expect( tpl.children[ 2 ] ).to.equal( v3 ); const rendered = tpl.render(); - expect( normalizeHtml( rendered.outerHTML ) ).to.equal( '

' ); + expect( normalizeHtml( rendered.outerHTML ) ) + .to.equal( '

' ); // Make sure the child views will not re–render their elements but // use ones rendered by the template instance above. expect( v1.element ).to.equal( rendered.firstChild ); - expect( v2.element ).to.equal( rendered.lastChild ); + expect( v2.element ).to.equal( rendered.children[ 1 ] ); + expect( v3.element ).to.equal( rendered.lastChild ); } ); it( 'renders view collection', () => { diff --git a/tests/view.js b/tests/view.js index b075d256..eebdffef 100644 --- a/tests/view.js +++ b/tests/view.js @@ -165,21 +165,6 @@ describe( 'View', () => { view.render(); expect( view.isRendered ).to.be.true; } ); - - it( 'calls render() on all view#_viewCollections', () => { - const view = new View(); - - const collectionA = view.createCollection(); - const collectionB = view.createCollection(); - - const spyA = testUtils.sinon.spy( collectionA, 'render' ); - const spyB = testUtils.sinon.spy( collectionB, 'render' ); - - view.render(); - sinon.assert.calledOnce( spyA ); - sinon.assert.calledOnce( spyB ); - sinon.assert.callOrder( spyA, spyB ); - } ); } ); describe( 'bind', () => { diff --git a/tests/viewcollection.js b/tests/viewcollection.js index e5a03f33..f7dc77f7 100644 --- a/tests/viewcollection.js +++ b/tests/viewcollection.js @@ -22,7 +22,6 @@ describe( 'ViewCollection', () => { describe( 'constructor()', () => { it( 'sets basic properties and attributes', () => { expect( collection.locale ).to.be.undefined; - expect( collection.ready ).to.be.false; expect( collection._parentElement ).to.be.null; expect( collection._idProperty ).to.equal( 'viewUid' ); } ); @@ -46,7 +45,6 @@ describe( 'ViewCollection', () => { } ).to.not.throw(); expect( () => { - collection.ready = true; collection.add( viewA ); collection.remove( viewA ); } ).to.not.throw(); @@ -110,8 +108,7 @@ describe( 'ViewCollection', () => { collection.add( viewC ); collection.remove( 1 ); - // Finally init the view. Check what's in there. - view.init(); + view.render(); expect( view.element.childNodes ).to.have.length( 2 ); expect( view.element.childNodes[ 0 ] ).to.equal( viewA.element ); @@ -120,45 +117,6 @@ describe( 'ViewCollection', () => { } ); } ); - describe( 'init()', () => { - it( 'should throw if already initialized', () => { - collection.init(); - - try { - collection.init(); - throw new Error( 'This should not be executed.' ); - } catch ( err ) { - expect( err ).to.be.instanceof( CKEditorError ); - expect( err.message ).to.match( /ui-viewcollection-init-reinit/ ); - } - } ); - - it( 'calls #init on all views in the collection', () => { - const viewA = new View(); - const viewB = new View(); - - viewA.element = document.createElement( 'a' ); - viewB.element = document.createElement( 'b' ); - - const spyA = testUtils.sinon.spy( viewA, 'init' ); - const spyB = testUtils.sinon.spy( viewB, 'init' ); - - collection.setParent( document.body ); - - collection.add( viewA ); - collection.add( viewB ); - - collection.init(); - sinon.assert.calledOnce( spyA ); - sinon.assert.calledOnce( spyB ); - sinon.assert.callOrder( spyA, spyB ); - - expect( viewA.element.parentNode ).to.equal( collection._parentElement ); - expect( viewA.element.nextSibling ).to.equal( viewB.element ); - expect( collection.ready ).to.be.true; - } ); - } ); - describe( 'destroy()', () => { it( 'calls #destroy on all views in the collection', () => { const viewA = new View(); @@ -178,26 +136,14 @@ describe( 'ViewCollection', () => { } ); describe( 'add()', () => { - it( 'initializes the new view in the collection', () => { - let view = new View(); - let spy = testUtils.sinon.spy( view, 'init' ); - - expect( collection.ready ).to.be.false; - expect( view.ready ).to.be.false; - - collection.add( view ); - expect( collection.ready ).to.be.false; - expect( view.ready ).to.be.false; - - sinon.assert.notCalled( spy ); - - view = new View(); - spy = testUtils.sinon.spy( view, 'init' ); + it( 'renders the new view in the collection', () => { + const view = new View(); + const spy = testUtils.sinon.spy( view, 'render' ); - collection.ready = true; + expect( view.isRendered ).to.be.false; collection.add( view ); - expect( view.ready ).to.be.true; + expect( view.isRendered ).to.be.true; sinon.assert.calledOnce( spy ); } ); From f08d48da2f0976383044f7769ecf2e6beb04a3e6 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 18 Oct 2017 11:34:11 +0200 Subject: [PATCH 04/44] Created View#setTemplate and View#extendTemplate methods. --- src/view.js | 30 ++++++++++++++++++++++++++++++ tests/view.js | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/src/view.js b/src/view.js index da7cfcba..755cea0b 100644 --- a/src/view.js +++ b/src/view.js @@ -369,6 +369,36 @@ export default class View { } } + /** + * Sets the {@link #template} of the view with with given definition. + * + * A shorthand for: + * + * view.template = new Template( definition ); + * + * @param {@link module:ui/template~TemplateDefinition} definition Definition of view's template. + */ + setTemplate( definition ) { + this.template = new Template( definition ); + } + + /** + * {@link module:ui/template~Template.extend Extends} the {@link #template} of the view with + * with given definition. + * + * A shorthand for: + * + * Template.extend( view.template, definition ); + * + * **Note**: Is requires the {@link #template} to be already set. See {@link #setTemplate}. + * + * @param {@link module:ui/template~TemplateDefinition} definition Definition which + * extends the {@link #template}. + */ + extendTemplate( definition ) { + Template.extend( this.template, definition ); + } + /** * Recursively renders the view. * diff --git a/tests/view.js b/tests/view.js index eebdffef..cae00671 100644 --- a/tests/view.js +++ b/tests/view.js @@ -11,6 +11,7 @@ import Template from '../src/template'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import Collection from '@ckeditor/ckeditor5-utils/src/collection'; import ViewCollection from '../src/viewcollection'; +import normalizeHtml from '@ckeditor/ckeditor5-utils/tests/_utils/normalizehtml'; let TestView, view, childA, childB; @@ -138,6 +139,53 @@ describe( 'View', () => { } ); } ); + describe( 'setTemplate()', () => { + it( 'sets the template', () => { + const view = new View(); + const bind = view.bindTemplate; + + view.set( 'foo', 'bar' ); + + view.setTemplate( { + tag: 'div', + attributes: { + class: [ + bind.to( 'foo' ) + ] + } + } ); + + view.render(); + + expect( normalizeHtml( view.element.outerHTML ) ).to.equal( '
' ); + } ); + } ); + + describe( 'extendTemplate()', () => { + it( 'extends the template', () => { + const view = new View(); + const bind = view.bindTemplate; + + view.set( 'foo', 'bar' ); + + view.setTemplate( { + tag: 'div' + } ); + + view.extendTemplate( { + attributes: { + class: [ + bind.to( 'foo' ) + ] + } + } ); + + view.render(); + + expect( normalizeHtml( view.element.outerHTML ) ).to.equal( '
' ); + } ); + } ); + describe( 'render()', () => { it( 'should throw if already rendered', () => { const view = new View(); From 2c8717dae96440b18af0402af8db310aaa9ba159 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 18 Oct 2017 11:46:56 +0200 Subject: [PATCH 05/44] Created View#setTemplate and View#extendTemplate methods. --- src/view.js | 18 +++++++++--------- tests/template.js | 4 ++-- tests/view.js | 14 +++++++------- tests/viewcollection.js | 5 ++--- 4 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/view.js b/src/view.js index 755cea0b..ecb10ca0 100644 --- a/src/view.js +++ b/src/view.js @@ -37,7 +37,7 @@ import isIterable from '@ckeditor/ckeditor5-utils/src/isiterable'; * // Views define their interface (state) using observable attributes. * this.set( 'elementClass', 'bar' ); * - * this.template = new Template( { + * this.setTemplate( { * tag: 'p', * * // The element of the view can be defined with its children. @@ -99,7 +99,7 @@ export default class View { * super(); * * // A template instance the #element will be created from. - * this.template = new Template( { + * this.setTemplate( { * tag: 'p' * * // ... @@ -211,7 +211,7 @@ export default class View { * isEnabled: true * } ); * - * this.template = new Template( { + * this.setTemplate( { * tag: 'p', * * attributes: { @@ -251,7 +251,7 @@ export default class View { * * this.items = this.createCollection(); * - * this.template = new Template( { + * this.setTemplate( { * tag: 'p', * * // `items` collection will render here. @@ -296,7 +296,7 @@ export default class View { * this.childA = new SomeChildView( locale ); * this.childB = new SomeChildView( locale ); * - * this.template = new Template( { tag: 'p' } ); + * this.setTemplate( { tag: 'p' } ); * * // Register the children. * this.registerChildren( [ this.childA, this.childB ] ); @@ -327,7 +327,7 @@ export default class View { * this.childA = new SomeChildView( locale ); * this.childB = new SomeChildView( locale ); * - * this.template = new Template( { + * this.setTemplate( { * tag: 'p', * * // These children will be added automatically. There's no @@ -374,7 +374,7 @@ export default class View { * * A shorthand for: * - * view.template = new Template( definition ); + * view.setTemplate( definition ); * * @param {@link module:ui/template~TemplateDefinition} definition Definition of view's template. */ @@ -421,7 +421,7 @@ export default class View { * * class SampleView extends View { * constructor() { - * this.template = new Template( { + * this.setTemplate( { * // ... * } ); * }, @@ -445,7 +445,7 @@ export default class View { * const view = new SampleView(); * * // Let's customize the view before it gets rendered. - * Template.extend( view.template, { + * view.extendTemplate( { * attributes: { * class: [ * 'additional-class' diff --git a/tests/template.js b/tests/template.js index e013e8a2..a46a474f 100644 --- a/tests/template.js +++ b/tests/template.js @@ -546,7 +546,7 @@ describe( 'Template', () => { // words to explain it. But what actually matters is that it proves the Template // class is free of "Maximum call stack size exceeded" error in certain // situations. - view.template = new Template( { + view.setTemplate( { tag: 'span', children: [ @@ -2972,7 +2972,7 @@ function dispatchEvent( el, domEvtName ) { function getView( def ) { const view = new View(); - view.template = new Template( def ); + view.setTemplate( def ); return view; } diff --git a/tests/view.js b/tests/view.js index cae00671..ea6a0f08 100644 --- a/tests/view.js +++ b/tests/view.js @@ -204,7 +204,7 @@ describe( 'View', () => { it( 'should set view#isRendered', () => { const view = new View(); - view.template = new Template( { + view.setTemplate( { tag: 'div' } ); @@ -266,11 +266,11 @@ describe( 'View', () => { const viewB = new View(); const viewC = new View(); - viewA.template = new Template( { tag: 'a' } ); - viewB.template = new Template( { tag: 'b' } ); - viewC.template = new Template( { tag: 'c' } ); + viewA.setTemplate( { tag: 'a' } ); + viewB.setTemplate( { tag: 'b' } ); + viewC.setTemplate( { tag: 'c' } ); - view.template = new Template( { + view.setTemplate( { tag: 'div', children: [ viewA, @@ -384,7 +384,7 @@ function setTestViewClass( templateDef ) { this.locale = { t() {} }; if ( templateDef ) { - this.template = new Template( templateDef ); + this.setTemplate( templateDef ); } } }; @@ -405,7 +405,7 @@ function createViewWithChildren() { constructor() { super(); - this.template = new Template( { + this.setTemplate( { tag: 'span' } ); } diff --git a/tests/viewcollection.js b/tests/viewcollection.js index f7dc77f7..7406ec93 100644 --- a/tests/viewcollection.js +++ b/tests/viewcollection.js @@ -9,7 +9,6 @@ import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import View from '../src/view'; import ViewCollection from '../src/viewcollection'; -import Template from '../src/template'; import normalizeHtml from '@ckeditor/ckeditor5-utils/tests/_utils/normalizehtml'; let collection; @@ -76,7 +75,7 @@ describe( 'ViewCollection', () => { function getView( text ) { const view = new View(); - view.template = new Template( { + view.setTemplate( { tag: 'li', children: [ { @@ -93,7 +92,7 @@ describe( 'ViewCollection', () => { collection.add( viewB ); // Put the collection in the template. - view.template = new Template( { + view.setTemplate( { tag: 'ul', children: collection } ); From 53c6647a73757114de13e29b3cea0886ecebfcd3 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 18 Oct 2017 12:18:34 +0200 Subject: [PATCH 06/44] Aligned ButtonView to the View#render API. Code refactoring in the component. --- src/button/buttonview.js | 95 ++++++++++++++++++++++++-------------- tests/button/buttonview.js | 39 ++++++++-------- 2 files changed, 82 insertions(+), 52 deletions(-) diff --git a/src/button/buttonview.js b/src/button/buttonview.js index ee1d8554..3ea95b01 100644 --- a/src/button/buttonview.js +++ b/src/button/buttonview.js @@ -8,7 +8,6 @@ */ import View from '../view'; -import Template from '../template'; import IconView from '../icon/iconview'; import TooltipView from '../tooltip/tooltipview'; @@ -26,7 +25,7 @@ import { getEnvKeystrokeText } from '@ckeditor/ckeditor5-utils/src/keyboard'; * withText: true * } ); * - * view.init(); + * view.render(); * * document.body.append( view.element ); * @@ -39,6 +38,8 @@ export default class ButtonView extends View { constructor( locale ) { super( locale ); + const bind = this.bindTemplate; + /** * The label of the button view visible to the user when {@link #withText} is `true`. * It can also be used to create a {@link #tooltip}. @@ -151,6 +152,30 @@ export default class ButtonView extends View { */ this.set( 'tabindex', -1 ); + /** + * Collection of the child views inside of the button {@link #element}. + * + * @readonly + * @member {module:ui/viewcollection~ViewCollection} + */ + this.children = this.createCollection(); + + /** + * Tooltip of the button view. It is configurable using the {@link #tooltip tooltip attribute}. + * + * @readonly + * @member {module:ui/tooltip/tooltipview~TooltipView} #tooltipView + */ + this.tooltipView = this._createTooltipView(); + + /** + * Label of the button view. It is configurable using the {@link #label label attribute}. + * + * @readonly + * @member {module:ui/view~View} #labelView + */ + this.labelView = this._createLabelView(); + /** * Tooltip of the button bound to the template. * @@ -167,14 +192,6 @@ export default class ButtonView extends View { this._getTooltipString.bind( this ) ); - /** - * Tooltip of the button view. It is configurable using the {@link #tooltip tooltip attribute}. - * - * @readonly - * @member {module:ui/tooltip/tooltipview~TooltipView} #tooltipView - */ - this.tooltipView = this._createTooltipView(); - /** * (Optional) The icon view of the button. Only present when the {@link #icon icon attribute} is defined. * @@ -182,9 +199,7 @@ export default class ButtonView extends View { * @member {module:ui/icon/iconview~IconView} #iconView */ - const bind = this.bindTemplate; - - this.template = new Template( { + this.setTemplate( { tag: 'button', attributes: { @@ -199,22 +214,7 @@ export default class ButtonView extends View { tabindex: bind.to( 'tabindex' ) }, - children: [ - { - tag: 'span', - - attributes: { - class: [ 'ck-button__label' ] - }, - - children: [ - { - text: bind.to( 'label' ) - } - ] - }, - this.tooltipView - ], + children: this.children, on: { mousedown: bind.to( evt => { @@ -246,18 +246,19 @@ export default class ButtonView extends View { /** * @inheritDoc */ - init() { + render() { + super.render(); + if ( this.icon ) { const iconView = this.iconView = new IconView(); iconView.bind( 'content' ).to( this, 'icon' ); - this.element.insertBefore( iconView.element, this.element.firstChild ); - // Make sure the icon will be destroyed along with the button. - this.addChildren( iconView ); + this.children.add( iconView ); } - super.init(); + this.children.add( this.tooltipView ); + this.children.add( this.labelView ); } /** @@ -283,6 +284,32 @@ export default class ButtonView extends View { return tooltipView; } + /** + * Creates a label view instance and binds it with button attributes. + * + * @private + * @returns {module:ui/view~View} + */ + _createLabelView() { + const labelView = new View(); + + labelView.setTemplate( { + tag: 'span', + + attributes: { + class: [ 'ck-button__label' ] + }, + + children: [ + { + text: this.bindTemplate.to( 'label' ) + } + ] + } ); + + return labelView; + } + /** * Gets the text for the {@link #tooltipView} from the combination of * {@link #tooltip}, {@link #label} and {@link #keystroke} attributes. diff --git a/tests/button/buttonview.js b/tests/button/buttonview.js index 17ea6419..5bb7da34 100644 --- a/tests/button/buttonview.js +++ b/tests/button/buttonview.js @@ -9,6 +9,8 @@ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import ButtonView from '../../src/button/buttonview'; import IconView from '../../src/icon/iconview'; import TooltipView from '../../src/tooltip/tooltipview'; +import View from '../../src/view'; +import ViewCollection from '../../src/viewcollection'; testUtils.createSinonSandbox(); @@ -18,7 +20,22 @@ describe( 'ButtonView', () => { beforeEach( () => { locale = { t() {} }; - return ( view = new ButtonView( locale ) ).init(); + view = new ButtonView( locale ); + view.render(); + } ); + + describe( 'constructor()', () => { + it( 'creates view#children collection', () => { + expect( view.children ).to.be.instanceOf( ViewCollection ); + } ); + + it( 'creates #tooltipView', () => { + expect( view.tooltipView ).to.be.instanceOf( TooltipView ); + } ); + + it( 'creates #labelView', () => { + expect( view.labelView ).to.be.instanceOf( View ); + } ); } ); describe( '