Bladeren bron

Major code refactoring, documentaion and tests for Region class.

Aleksander Nowodzinski 10 jaren geleden
bovenliggende
commit
f32e1dd0dc
2 gewijzigde bestanden met toevoegingen van 100 en 151 verwijderingen
  1. 28 97
      packages/ckeditor5-ui/src/ui/region.js
  2. 72 54
      packages/ckeditor5-ui/tests/ui/region.js

+ 28 - 97
packages/ckeditor5-ui/src/ui/region.js

@@ -14,9 +14,8 @@
 
 CKEDITOR.define( [
 	'collection',
-	'model',
-	'ckeditorerror',
-], ( Collection, Model, CKEditorError ) => {
+	'model'
+], ( Collection, Model ) => {
 	class Region extends Model {
 		/**
 		 * Creates an instance of the {@link Region} class.
@@ -25,132 +24,64 @@ CKEDITOR.define( [
 		 * @param {HTMLElement} [el] The element used for this region.
 		 * @constructor
 		 */
-		constructor( name, elDef ) {
+		constructor( name ) {
 			super();
 
 			/**
 			 * The name of the region.
+			 *
+			 * @property {String}
 			 */
 			this.name = name;
 
 			/**
-			 * @property {HTMLElement} _elDef
-			 */
-			this._elDef = elDef;
-
-			/**
 			 * Views which belong to the region.
+			 *
+			 * @property {Collection}
 			 */
 			this.views = new Collection();
 
 			/**
-			 * @property {View} parent
+			 * Element of this region (see {@link #init}).
+			 *
+			 * @property {HTMLElement}
 			 */
+			this.el = null;
 		}
 
 		/**
 		 * @param
 		 * @returns
 		 */
-		init( parent ) {
-			this.parent = parent;
-
-			if ( this.el ) {
-				this._initChildViews();
-			}
-		}
-
-		/**
-		 * Element of this Region. The element is rendered on first reference.
-		 *
-		 * @property el
-		 */
-		get el() {
-			return this._el || this._getElement();
-		}
-
-		set el( el ) {
-			this._el = el;
-		}
-
-		/**
-		 * @param
-		 * @returns
-		 */
-		_getElement() {
-			const elDef = this._elDef;
-			let el;
-
-			if ( typeof elDef == 'string' ) {
-				el = this.parent.el.querySelector( elDef );
-			} else if ( typeof elDef == 'function' ) {
-				el = elDef( this.parent.el );
-			} else if ( elDef === true ) {
-				el = null;
-			} else {
-				/**
-				 * Region definition must be either `Function`, `String` or `Boolean` (`true`).
-				 *
-				 * @error ui-region-element
-				 * @param {Region} region
-				 */
-				throw new CKEditorError(
-					'ui-region-element: Region definition must be either `Function`, `String` or `Boolean` (`true`).',
-					{ region: this }
-				);
-			}
-
-			return ( this._el = el );
-		}
+		init( regionEl ) {
+			this.el = regionEl;
 
-		/**
-		 * @param
-		 * @returns
-		 */
-		_initChildViews() {
-			let view;
+			if ( regionEl ) {
+				this.views.on( 'add', ( evt, childView, index ) => {
+					regionEl.insertBefore( childView.el, regionEl.childNodes[ index + 1 ] );
+				} );
 
-			// Add registered views to DOM.
-			for ( view of this.views ) {
-				this.el.appendChild( view.el );
+				this.views.on( 'remove', ( evt, childView ) => {
+					childView.el.remove();
+				} );
 			}
-
-			// Attach listeners for future manipulation.
-			this.views.on( 'add', ( evt, view ) => {
-				if ( this.el ) {
-					this.el.appendChild( view.el );
-				}
-			} );
-
-			this.views.on( 'remove', ( evt, view ) => {
-				view.el.remove();
-			} );
-		}
-
-		/**
-		 * @param
-		 * @returns
-		 */
-		addChild( view, index ) {
-			this.views.add( view, index );
-		}
-
-		/**
-		 * @param
-		 * @returns
-		 */
-		removeChild( view ) {
-			this.views.remove( view );
 		}
 
 		/**
 		 * Destroys the Region instance.
 		 */
 		destroy() {
+			if ( this.el ) {
+				for ( let view of this.views ) {
+					view.el.remove();
+					this.views.remove( view );
+				}
+			}
+
 			// Drop the reference to HTMLElement but don't remove it from DOM.
 			// Element comes as a parameter and it could be a part of the View.
 			// Then it's up to the View what to do with it when the View is destroyed.
-			this.el = null;
+			this.el = this.views = null;
 		}
 	}
 

+ 72 - 54
packages/ckeditor5-ui/tests/ui/region.js

@@ -12,86 +12,103 @@ const modules = bender.amd.require( 'ckeditor', 'ui/region', 'ui/view', 'collect
 
 bender.tools.createSinonSandbox();
 
+let Region, View;
 let TestViewA, TestViewB;
 let region, el;
 
-beforeEach( createRegionInstance );
+describe( 'View', () => {
+	beforeEach( createRegionInstance );
 
-describe( 'constructor', () => {
-	it( 'accepts name and element', () => {
-		expect( region ).to.have.property( 'name', 'foo' );
-		expect( region ).to.have.property( 'el', el );
+	describe( 'constructor', () => {
+		it( 'accepts name', () => {
+			expect( region.name ).to.be.equal( 'foo' );
+			expect( region.el ).to.be.null;
+			expect( region.views.length ).to.be.equal( 0 );
+		} );
 	} );
-} );
 
-describe( 'views collection', () => {
-	it( 'is an instance of Collection', () => {
-		const Collection = modules.collection;
-		expect( region.views ).to.be.an.instanceof( Collection );
+	describe( 'init', () => {
+		it( 'accepts region element', () => {
+			region.init( el );
+
+			expect( region.el ).to.be.equal( el );
+		} );
 	} );
 
-	it( 'updates DOM when adding views', () => {
-		expect( region.el.childNodes.length ).to.be.equal( 0 );
+	describe( 'views collection', () => {
+		it( 'updates DOM when adding views', () => {
+			region.init( el );
 
-		region.views.add( new TestViewA() );
-		expect( region.el.childNodes.length ).to.be.equal( 1 );
+			expect( region.el.childNodes.length ).to.be.equal( 0 );
 
-		region.views.add( new TestViewA() );
-		expect( region.el.childNodes.length ).to.be.equal( 2 );
-	} );
+			region.views.add( new TestViewA() );
+			expect( region.el.childNodes.length ).to.be.equal( 1 );
 
-	it( 'updates DOM when removing views', () => {
-		let viewA = new TestViewA();
-		let viewB = new TestViewB();
+			region.views.add( new TestViewA() );
+			expect( region.el.childNodes.length ).to.be.equal( 2 );
+		} );
 
-		region.views.add( viewA );
-		region.views.add( viewB );
+		it( 'does not update DOM when no region element', () => {
+			region.init();
 
-		expect( el.childNodes.length ).to.be.equal( 2 );
-		expect( el.firstChild.nodeName ).to.be.equal( 'A' );
-		expect( el.lastChild.nodeName ).to.be.equal( 'B' );
+			expect( () => {
+				region.views.add( new TestViewA() );
+				region.views.add( new TestViewA() );
+			} ).to.not.throw();
+		} );
 
-		region.views.remove( viewA );
-		expect( el.childNodes.length ).to.be.equal( 1 );
-		expect( el.firstChild.nodeName ).to.be.equal( 'B' );
+		it( 'updates DOM when removing views', () => {
+			region.init( el );
 
-		region.views.remove( viewB );
-		expect( el.childNodes.length ).to.be.equal( 0 );
-	} );
-} );
+			let viewA = new TestViewA();
+			let viewB = new TestViewB();
 
-describe( 'destroy', () => {
-	it( 'destroys the region', () => {
-		// Append the region's element to some container.
-		let container = document.createElement( 'div' );
-		container.appendChild( el );
-		expect( el.parentNode ).to.be.equal( container );
+			region.views.add( viewA );
+			region.views.add( viewB );
 
-		region.destroy();
+			expect( el.childNodes.length ).to.be.equal( 2 );
+			expect( el.firstChild.nodeName ).to.be.equal( 'A' );
+			expect( el.lastChild.nodeName ).to.be.equal( 'B' );
 
-		// Make sure destruction of the region does affect passed element.
-		expect( el.parentNode ).to.be.equal( container );
-		expect( region.el ).to.be.null;
+			region.views.remove( viewA );
+			expect( el.childNodes.length ).to.be.equal( 1 );
+			expect( el.firstChild.nodeName ).to.be.equal( 'B' );
+
+			region.views.remove( viewB );
+			expect( el.childNodes.length ).to.be.equal( 0 );
+		} );
 	} );
 
-	it( 'destroys children views', () => {
-		let view = new TestViewA();
-		let spy = bender.sinon.spy( view, 'destroy' );
+	describe( 'destroy', () => {
+		it( 'destroys the region', () => {
+			region.init( el );
+			region.views.add( new TestViewA() );
+
+			const elRef = region.el;
+			const viewsRef = region.views;
 
-		// Append the view to the region.
-		region.views.add( view );
-		expect( region.views ).to.have.length( 1 );
+			region.destroy();
 
-		region.destroy();
+			expect( elRef.parentNode ).to.be.null;
+			expect( region.name ).to.be.equal( 'foo' );
+			expect( region.views ).to.be.null;
+			expect( viewsRef.length ).to.be.equal( 0 );
+			expect( region.el ).to.be.null;
+		} );
 
-		expect( region.views ).to.have.length( 0 );
-		expect( spy.calledOnce ).to.be.true;
+		it( 'destroys an element–less region', () => {
+			region.init();
+
+			expect( () => {
+				region.destroy();
+			} ).to.not.throw();
+		} );
 	} );
 } );
 
 function createRegionInstance() {
-	const Region = modules[ 'ui/region' ];
-	const View = modules[ 'ui/view' ];
+	Region = modules[ 'ui/region' ];
+	View = modules[ 'ui/view' ];
 
 	class A extends View {
 		constructor() {
@@ -111,5 +128,6 @@ function createRegionInstance() {
 	TestViewB = B;
 
 	el = document.createElement( 'div' );
-	region = new Region( 'foo', el );
+
+	region = new Region( 'foo' );
 }