Merge pull request #67 from ckeditor/t/66

Fix: Destroying `FileDialogButtonView` should not throw an error. Closes #66. BREAKING CHANGE: The `FileDialogButtonView` is not a `ButtonView` instance anymore but a wrapper instead. The button of the component is available under the `#buttonView` property.
ckeditor · Oct 10, 2017 · 2d4ba62 · 2d4ba62
2 parents 811d32f + 4134934
commit 2d4ba62
Show file tree

Hide file tree

Showing 3 changed files with 114 additions and 65 deletions.
diff --git a/src/imageuploadbutton.js b/src/imageuploadbutton.js
@@ -40,13 +40,16 @@ export default class ImageUploadButton extends Plugin {
 			const command = editor.commands.get( 'imageUpload' );
 
 			view.set( {
-				label: t( 'Insert image' ),
-				icon: imageIcon,
-				tooltip: true,
 				acceptedType: 'image/*',
 				allowMultipleFiles: true
 			} );
 
+			view.buttonView.set( {
+				label: t( 'Insert image' ),
+				icon: imageIcon,
+				tooltip: true
+			} );
+
 			view.bind( 'isEnabled' ).to( command );
 
 			view.on( 'done', ( evt, files ) => {

diff --git a/src/ui/filedialogbuttonview.js b/src/ui/filedialogbuttonview.js
@@ -3,8 +3,6 @@
  * For licensing, see LICENSE.md.
  */
 
-/* globals document */
-
 /**
  * @module upload/ui/filedialogbuttonview
  */
@@ -14,24 +12,53 @@ import View from '@ckeditor/ckeditor5-ui/src/view';
 import Template from '@ckeditor/ckeditor5-ui/src/template';
 
 /**
- * File Dialog button view.
+ * The file dialog button view.
+ *
+ * This component provides a button that opens the native file selection dialog.
+ * It can be used to implement the UI of a file upload feature.
+ *
+ *		const view = new FileDialogButtonView( locale );
+ *
+ *		view.set( {
+ *			acceptedType: 'image/*',
+ *			allowMultipleFiles: true
+ *		} );
+ *
+ *		view.buttonView.set( {
+ *			label: t( 'Insert image' ),
+ *			icon: imageIcon,
+ *			tooltip: true
+ *		} );
  *
- * @extends module:ui/button/buttonview~ButtonView
+ *		view.on( 'done', ( evt, files ) => {
+ *			for ( const file of Array.from( files ) ) {
+ *				console.log( 'Selected file', file );
+ *			}
+ *		} );
+ *
+ * @extends module:ui/view~View
  */
-export default class FileDialogButtonView extends ButtonView {
+export default class FileDialogButtonView extends View {
 	/**
 	 * @inheritDoc
 	 */
 	constructor( locale ) {
 		super( locale );
 
 		/**
-		 * Hidden input view used to execute file dialog. It will be hidden and added to the end of `document.body`.
+		 * The button view of the component.
+		 *
+		 * @member {module:ui/button/buttonview~ButtonView}
+		 */
+		this.buttonView = new ButtonView( locale );
+
+		/**
+		 * A hidden `<input>` view used to execute file dialog.
 		 *
 		 * @protected
 		 * @member {module:upload/ui/filedialogbuttonview~FileInputView}
 		 */
-		this.fileInputView = new FileInputView( locale );
+		this._fileInputView = new FileInputView( locale );
 
 		/**
 		 * Accepted file types. Can be provided in form of file extensions, media type or one of:
@@ -42,50 +69,56 @@ export default class FileDialogButtonView extends ButtonView {
 		 * @observable
 		 * @member {String} #acceptedType
 		 */
-		this.fileInputView.bind( 'acceptedType' ).to( this, 'acceptedType' );
+		this._fileInputView.bind( 'acceptedType' ).to( this );
 
 		/**
 		 * Indicates if multiple files can be selected. Defaults to `true`.
 		 *
 		 * @observable
 		 * @member {Boolean} #allowMultipleFiles
 		 */
-		this.set( 'allowMultipleFiles', false );
-		this.fileInputView.bind( 'allowMultipleFiles' ).to( this, 'allowMultipleFiles' );
+		this._fileInputView.bind( 'allowMultipleFiles' ).to( this );
 
 		/**
 		 * Fired when file dialog is closed with file selected.
 		 *
-		 *	fileDialogButtonView.on( 'done', ( evt, files ) => {
-		 *		for ( const file of files ) {
-		 *			processFile( file );
+		 *		view.on( 'done', ( evt, files ) => {
+		 *			for ( const file of files ) {
+		 *				console.log( 'Selected file', file );
+		 *			}
 		 *		}
-		 *	}
 		 *
 		 * @event done
 		 * @param {Array.<File>} files Array of selected files.
 		 */
-		this.fileInputView.delegate( 'done' ).to( this );
+		this._fileInputView.delegate( 'done' ).to( this );
 
-		this.on( 'execute', () => {
-			this.fileInputView.open();
+		this.template = new Template( {
+			tag: 'span',
+			attributes: {
+				class: 'ck-file-dialog-button',
+			},
+			children: [
+				this.buttonView,
+				this._fileInputView
+			]
 		} );
 
-		document.body.appendChild( this.fileInputView.element );
+		this.buttonView.on( 'execute', () => {
+			this._fileInputView.open();
+		} );
 	}
 
 	/**
-	 * @inheritDoc
+	 * Focuses the {@link #buttonView}.
 	 */
-	destroy() {
-		document.body.removeChild( this.fileInputView.element );
-
-		super.destroy();
+	focus() {
+		this.buttonView.focus();
 	}
 }
 
 /**
- * Hidden file input view class.
+ * The hidden file input view class.
  *
  * @private
  * @extends {module:ui/view~View}

diff --git a/tests/ui/filedialogbuttonview.js b/tests/ui/filedialogbuttonview.js
@@ -3,64 +3,77 @@
  * For licensing, see LICENSE.md.
  */
 
-/* globals document */
-
-import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
 import FileDialogButtonView from '../../src/ui/filedialogbuttonview';
+import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview';
+import View from '@ckeditor/ckeditor5-ui/src/view';
 
 describe( 'FileDialogButtonView', () => {
-	let view, editor;
+	let view, localeMock;
 
 	beforeEach( () => {
-		const editorElement = document.createElement( 'div' );
-		document.body.appendChild( editorElement );
-
-		return ClassicEditor
-			.create( editorElement )
-			.then( newEditor => {
-				editor = newEditor;
+		localeMock = { t: val => val };
+		view = new FileDialogButtonView( localeMock );
 
-				view = new FileDialogButtonView( editor.locale );
-			} );
+		return view.init();
 	} );
 
-	it( 'should append input view to document body', () => {
-		expect( view.fileInputView.element.parentNode ).to.equal( document.body );
+	it( 'should be rendered from a template', () => {
+		expect( view.element.classList.contains( 'ck-file-dialog-button' ) ).to.true;
 	} );
 
-	it( 'should remove input view from body after destroy', () => {
-		view.destroy();
+	describe( 'child views', () => {
+		describe( 'button view', () => {
+			it( 'should be rendered', () => {
+				expect( view.buttonView ).to.instanceof( ButtonView );
+				expect( view.buttonView ).to.equal( view.template.children.get( 0 ) );
+			} );
 
-		expect( view.fileInputView.element.parentNode ).to.be.null;
-	} );
+			it( 'should open file dialog on execute', () => {
+				const spy = sinon.spy( view._fileInputView, 'open' );
+				view.buttonView.fire( 'execute' );
 
-	it( 'should open file dialog on execute', () => {
-		const spy = sinon.spy( view.fileInputView, 'open' );
-		view.fire( 'execute' );
+				sinon.assert.calledOnce( spy );
+			} );
+		} );
 
-		sinon.assert.calledOnce( spy );
-	} );
+		describe( 'file dialog', () => {
+			it( 'should be rendered', () => {
+				expect( view._fileInputView ).to.instanceof( View );
+				expect( view._fileInputView ).to.equal( view.template.children.get( 1 ) );
+			} );
 
-	it( 'should pass acceptedType to input view', () => {
-		view.set( { acceptedType: 'audio/*' } );
+			it( 'should be bound to view#acceptedType', () => {
+				view.set( { acceptedType: 'audio/*' } );
 
-		expect( view.fileInputView.acceptedType ).to.equal( 'audio/*' );
-	} );
+				expect( view._fileInputView.acceptedType ).to.equal( 'audio/*' );
+			} );
 
-	it( 'should pass allowMultipleFiles to input view', () => {
-		view.set( { allowMultipleFiles: true } );
+			it( 'should be bound to view#allowMultipleFiles', () => {
+				view.set( { allowMultipleFiles: true } );
 
-		expect( view.fileInputView.allowMultipleFiles ).to.be.true;
-	} );
+				expect( view._fileInputView.allowMultipleFiles ).to.be.true;
+			} );
 
-	it( 'should delegate input view done event', done => {
-		const files = [];
+			it( 'should delegate done event to view', () => {
+				const spy = sinon.spy();
+				const files = [];
 
-		view.on( 'done', ( evt, data ) => {
-			expect( data ).to.equal( files );
-			done();
+				view.on( 'done', spy );
+				view._fileInputView.fire( 'done', files );
+
+				sinon.assert.calledOnce( spy );
+				expect( spy.lastCall.args[ 1 ] ).to.equal( files );
+			} );
 		} );
+	} );
+
+	describe( 'focus()', () => {
+		it( 'should focus view#buttonView', () => {
+			const spy = sinon.spy( view.buttonView, 'focus' );
 
-		view.fileInputView.fire( 'done', files );
+			view.focus();
+
+			sinon.assert.calledOnce( spy );
+		} );
 	} );
 } );