Ver código fonte

Merge pull request #51 from ckeditor/t/47

t/47: A new approach to falsy values in Template definition
Oskar Wróbel 9 anos atrás
pai
commit
1b7affd2ea

+ 41 - 28
packages/ckeditor5-ui/src/template.js

@@ -9,6 +9,7 @@ import CKEditorError from '../utils/ckeditorerror.js';
 import mix from '../utils/mix.js';
 import EmitterMixin from '../utils/emittermixin.js';
 import cloneDeepWith from '../utils/lib/lodash/cloneDeepWith.js';
+import isObject from '../utils/lib/lodash/isObject.js';
 
 const bindToSymbol = Symbol( 'bindTo' );
 const bindIfSymbol = Symbol( 'bindIf' );
@@ -401,17 +402,20 @@ export default class Template {
 
 		for ( attrName in attributes ) {
 			attrValue = attributes[ attrName ];
-			attrNs = attrValue[ 0 ].ns || null;
+
+			// Detect custom namespace:
+			// 		{ class: { ns: 'abc', value: Template.bind( ... ).to( ... ) } }
+			attrNs = isObject( attrValue[ 0 ] ) && attrValue[ 0 ].ns ? attrValue[ 0 ].ns : null;
 
 			// Activate binding if one is found. Cases:
 			// 		{ class: [ Template.bind( ... ).to( ... ) ] }
 			// 		{ class: [ 'bar', Template.bind( ... ).to( ... ), 'baz' ] }
 			// 		{ class: { ns: 'abc', value: Template.bind( ... ).to( ... ) } }
 			if ( hasBinding( attrValue ) ) {
-				// Normalize attributes with additional data like namespace:
-				//		{ class: { ns: 'abc', value: [ ... ] } }
 				this._bindToObservable(
-					attrValue[ 0 ].value || attrValue,
+					// Normalize attributes with additional data like namespace:
+					//		{ class: { ns: 'abc', value: [ ... ] } }
+					attrNs ? attrValue[ 0 ].value : attrValue,
 					el,
 					getAttributeUpdater( el, attrName, attrNs )
 				);
@@ -437,9 +441,11 @@ export default class Template {
 					// Flatten the array.
 					.reduce( ( p, n ) => p.concat( n ), [] )
 					// Convert into string.
-					.reduce( arrayValueReducer );
+					.reduce( arrayValueReducer, '' );
 
-				el.setAttributeNS( attrNs, attrName, attrValue );
+				if ( !isFalsy( attrValue ) ) {
+					el.setAttributeNS( attrNs, attrName, attrValue );
+				}
 			}
 		}
 	}
@@ -553,9 +559,11 @@ export default class Template {
 	 */
 	_bindToObservable( valueSchema ) {
 		valueSchema
-			// Filter inactive bindings from schema, like static strings, etc.
+			// Filter "falsy" (false, undefined, null, '') value schema components out.
+			.filter( item => !isFalsy( item ) )
+			// Filter inactive bindings from schema, like static strings ('foo'), numbers (42), etc.
 			.filter( item => item.observable )
-			// Let the emitter listen to observable change:attribute event.
+			// Once only the actual binding are left, let the emitter listen to observable change:attribute event.
 			// TODO: Reduce the number of listeners attached as many bindings may listen
 			// to the same observable attribute.
 			.forEach( ( { emitter, observable, attribute } ) => {
@@ -605,9 +613,9 @@ function hasBinding( valueSchema ) {
 // @return {Array}
 function getBindingValue( valueSchema, domNode ) {
 	return valueSchema.map( schemaItem => {
-		let { observable, callback, type } = schemaItem;
-
-		if ( observable ) {
+		// Process {@link ui.TemplateBinding} bindings.
+		if ( isObject( schemaItem ) ) {
+			let { observable, callback, type } = schemaItem;
 			let modelValue = observable[ schemaItem.attribute ];
 
 			// Process the value with the callback.
@@ -616,13 +624,14 @@ function getBindingValue( valueSchema, domNode ) {
 			}
 
 			if ( type === bindIfSymbol ) {
-				return !!modelValue ? schemaItem.valueIfTrue || true : '';
+				return isFalsy( modelValue ) ? false : ( schemaItem.valueIfTrue || true );
 			} else {
 				return modelValue;
 			}
-		} else {
-			return schemaItem;
 		}
+
+		// All static values like strings, numbers, and "falsy" values (false, null, undefined, '', etc.) just pass.
+		return schemaItem;
 	} );
 }
 
@@ -634,26 +643,19 @@ function getBindingValue( valueSchema, domNode ) {
 // @param {Function} domUpdater A function which updates DOM (like attribute or text).
 function syncBinding( valueSchema, domNode, domUpdater ) {
 	let value = getBindingValue( valueSchema, domNode );
-	let shouldSet;
 
 	// Check if valueSchema is a single Template.bind.if, like:
 	//		{ class: Template.bind.if( 'foo' ) }
 	if ( valueSchema.length == 1 && valueSchema[ 0 ].type == bindIfSymbol ) {
 		value = value[ 0 ];
-		shouldSet = value !== '';
-
-		if ( shouldSet ) {
-			value = value === true ? '' : value;
-		}
 	} else {
 		value = value.reduce( arrayValueReducer, '' );
-		shouldSet = value;
 	}
 
-	if ( shouldSet ) {
-		domUpdater.set( value );
-	} else {
+	if ( isFalsy( value ) ) {
 		domUpdater.remove();
+	} else {
+		domUpdater.set( value );
 	}
 }
 
@@ -889,10 +891,13 @@ function arrayify( obj, key ) {
 // @param {String} cur
 // @returns {String}
 function arrayValueReducer( prev, cur ) {
-	return prev === '' ?
-			`${cur}`
-		:
-			cur === '' ? `${prev}` : `${prev} ${cur}`;
+	if ( isFalsy( cur ) ) {
+		return prev;
+	} else if ( isFalsy( prev ) )  {
+		return cur;
+	} else {
+		return `${prev} ${cur}`;
+	}
 }
 
 // Extends one object defined in the following format:
@@ -961,6 +966,14 @@ function extendTemplateDefinition( def, extDef ) {
 	}
 }
 
+// Checks if value is "falsy".
+// Note: 0 (Number) is not "falsy" in this context.
+//
+// @param {*} value Value to be checked.
+function isFalsy( value ) {
+	return !value && value !== 0;
+}
+
 /**
  * A definition of {@link ui.Template}.
  * See: {@link ui.TemplateValueSchema}.

+ 1 - 1
packages/ckeditor5-ui/tests/editableui/editableuiview.js

@@ -68,7 +68,7 @@ describe( 'EditableUIView', () => {
 			it( 'reacts on editable.isReadOnly', () => {
 				editable.isReadOnly = true;
 
-				expect( view.element.getAttribute( 'contenteditable' ) ).to.equal( 'false' );
+				expect( view.element.hasAttribute( 'contenteditable' ) ).to.be.false;
 			} );
 		} );
 	} );

+ 68 - 24
packages/ckeditor5-ui/tests/template.js

@@ -165,7 +165,7 @@ describe( 'Template', () => {
 				children: [ 'foo' ]
 			} ).render();
 
-			expect( el.outerHTML ).to.be.equal( '<p class="" x="">foo</p>' );
+			expect( el.outerHTML ).to.be.equal( '<p>foo</p>' );
 		} );
 
 		it( 'renders HTMLElement attributes – falsy values', () => {
@@ -173,12 +173,13 @@ describe( 'Template', () => {
 				tag: 'p',
 				attributes: {
 					class: false,
-					x: [ '', null ]
+					x: [ '', null, undefined, false ],
+					y: [ 'foo', null, undefined, false ]
 				},
 				children: [ 'foo' ]
 			} ).render();
 
-			expect( el.outerHTML ).to.be.equal( '<p class="false" x="null">foo</p>' );
+			expect( el.outerHTML ).to.be.equal( '<p y="foo">foo</p>' );
 		} );
 
 		it( 'renders HTMLElement attributes in a custom namespace', () => {
@@ -427,7 +428,8 @@ describe( 'Template', () => {
 						attributes: {
 							style: {
 								width: null,
-								height: false
+								height: false,
+								color: undefined
 							}
 						}
 					} );
@@ -988,7 +990,7 @@ describe( 'Template', () => {
 					} );
 
 					observable.foo = 'moo';
-					expect( el.outerHTML ).to.equal( '<p class="undefined">moo</p>' );
+					expect( el.outerHTML ).to.equal( '<p>moo</p>' );
 
 					observable.foo = 'changed';
 					expect( el.outerHTML ).to.equal( '<p class="changed">changed</p>' );
@@ -1049,28 +1051,31 @@ describe( 'Template', () => {
 					setElement( {
 						tag: 'p',
 						attributes: {
-							'class': bind.to( 'foo' )
+							simple: bind.to( 'foo' ),
+							complex: [ null, bind.to( 'foo' ), undefined, false ],
+							zero: [ 0, bind.to( 'foo' ) ],
+							emptystring: [ '', bind.to( 'foo' ) ]
 						},
 						children: [ 'abc' ]
 					} );
 
 					observable.foo = 'bar';
-					expect( el.outerHTML ).to.equal( '<p class="bar">abc</p>' );
+					expect( el.outerHTML ).to.equal( '<p simple="bar" complex="bar" zero="0 bar" emptystring="bar">abc</p>' );
+
+					observable.foo = 0;
+					expect( el.outerHTML ).to.equal( '<p simple="0" complex="0" zero="0 0" emptystring="0">abc</p>' );
 
 					observable.foo = false;
-					expect( el.outerHTML ).to.equal( '<p class="false">abc</p>' );
+					expect( el.outerHTML ).to.equal( '<p zero="0">abc</p>' );
 
 					observable.foo = null;
-					expect( el.outerHTML ).to.equal( '<p class="null">abc</p>' );
+					expect( el.outerHTML ).to.equal( '<p zero="0">abc</p>' );
 
 					observable.foo = undefined;
-					expect( el.outerHTML ).to.equal( '<p class="undefined">abc</p>' );
-
-					observable.foo = 0;
-					expect( el.outerHTML ).to.equal( '<p class="0">abc</p>' );
+					expect( el.outerHTML ).to.equal( '<p zero="0">abc</p>' );
 
 					observable.foo = '';
-					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
+					expect( el.outerHTML ).to.equal( '<p zero="0">abc</p>' );
 				} );
 
 				it( 'allows binding attribute to the observable – a custom namespace', () => {
@@ -1124,14 +1129,26 @@ describe( 'Template', () => {
 						children: [ 'abc' ]
 					} );
 
+					observable.foo = 'bar';
+					expect( el.outerHTML ).to.equal( '<p class="true">abc</p>' );
+
 					observable.foo = true;
-					expect( el.outerHTML ).to.equal( '<p class="">abc</p>' );
+					expect( el.outerHTML ).to.equal( '<p class="true">abc</p>' );
+
+					observable.foo = 0;
+					expect( el.outerHTML ).to.equal( '<p class="true">abc</p>' );
 
 					observable.foo = false;
 					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
 
-					observable.foo = 'bar';
-					expect( el.outerHTML ).to.equal( '<p class="">abc</p>' );
+					observable.foo = null;
+					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
+
+					observable.foo = undefined;
+					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
+
+					observable.foo = '';
+					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
 				} );
 
 				// TODO: Is this alright? It makes sense but it's pretty useless. Text Node cannot be
@@ -1146,13 +1163,25 @@ describe( 'Template', () => {
 						]
 					} );
 
+					observable.foo = 'abc';
+					expect( el.outerHTML ).to.equal( '<p>true</p>' );
+
 					observable.foo = true;
-					expect( el.outerHTML ).to.equal( '<p></p>' );
+					expect( el.outerHTML ).to.equal( '<p>true</p>' );
+
+					observable.foo = 0;
+					expect( el.outerHTML ).to.equal( '<p>true</p>' );
 
 					observable.foo = false;
 					expect( el.outerHTML ).to.equal( '<p></p>' );
 
-					observable.foo = 'bar';
+					observable.foo = null;
+					expect( el.outerHTML ).to.equal( '<p></p>' );
+
+					observable.foo = undefined;
+					expect( el.outerHTML ).to.equal( '<p></p>' );
+
+					observable.foo = '';
 					expect( el.outerHTML ).to.equal( '<p></p>' );
 				} );
 
@@ -1168,11 +1197,23 @@ describe( 'Template', () => {
 					observable.foo = 'bar';
 					expect( el.outerHTML ).to.equal( '<p class="bar">abc</p>' );
 
-					observable.foo = false;
-					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
+					observable.foo = true;
+					expect( el.outerHTML ).to.equal( '<p class="bar">abc</p>' );
+
+					observable.foo = 0;
+					expect( el.outerHTML ).to.equal( '<p class="bar">abc</p>' );
 
 					observable.foo = 64;
 					expect( el.outerHTML ).to.equal( '<p class="bar">abc</p>' );
+
+					observable.foo = false;
+					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
+
+					observable.foo = null;
+					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
+
+					observable.foo = undefined;
+					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
 				} );
 
 				it( 'allows binding attribute to the observable – value of an attribute (Text Node)', () => {
@@ -1266,6 +1307,12 @@ describe( 'Template', () => {
 					observable.foo = 'bar';
 					expect( el.outerHTML ).to.equal( '<p class="foo-is-set">abc</p>' );
 
+					observable.foo = true;
+					expect( el.outerHTML ).to.equal( '<p class="foo-is-set">abc</p>' );
+
+					observable.foo = 0;
+					expect( el.outerHTML ).to.equal( '<p class="foo-is-set">abc</p>' );
+
 					observable.foo = false;
 					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
 
@@ -1277,9 +1324,6 @@ describe( 'Template', () => {
 
 					observable.foo = '';
 					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
-
-					observable.foo = 0;
-					expect( el.outerHTML ).to.equal( '<p>abc</p>' );
 				} );
 			} );