Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Feature: Introduced PendingActions plugin. #127

Merged
merged 18 commits into from
May 15, 2018
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
178 changes: 178 additions & 0 deletions src/pendingactions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
/**
* @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/**
* @module core/pendingactions
*/

/* global window */

import Plugin from './plugin';
import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
import DOMEmitterMixin from '@ckeditor/ckeditor5-utils/src/dom/emittermixin';
import Collection from '@ckeditor/ckeditor5-utils/src/collection';
import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';

// This plugin is not able to interrupt actions or manage that in any other way.

/**
* List of editor pending actions.
*
* This plugin should be used to synchronise plugins that execute long-lasting actions
* (i.e. file upload) with the editor integration. It gives a developer, who integrates the editor,
* an easy way to check if there are any pending action whenever such information is needed.
* All plugins, which register pending action provides also a message what action is ongoing
* which can be displayed to a user and let him decide if he wants to interrupt the action or wait.
*
* This plugin listens to `window#beforeunload` event and displays browser prompt when a pending action is in progress.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not anymore.

*
* @extends module:core/plugin~Plugin
*/
export default class PendingActions extends Plugin {
/**
* @inheritDoc
*/
static get pluginName() {
return 'PendingActions';
}

/**
* @inheritDoc
*/
constructor( editor ) {
super( editor );

/**
* DOM Emitter.
*
* @private
* @type {module:utils/dom/emittermixin~EmitterMixin}
*/
this._domEmitter = Object.create( DOMEmitterMixin );
}

/**
* @inheritDoc
*/
init() {
/**
* Defines whether there is any registered pending action or not.
*
* @readonly
* @observable
* @type {Boolean} #isPending
*/
this.set( 'isPending', false );

/**
* List of pending actions.
*
* @private
* @type {module:utils/collection~Collection}
*/
this._actions = new Collection( { idProperty: '_id' } );
this._actions.delegate( 'add', 'remove' ).to( this );

// It's not possible to easy test it because karma uses `beforeunload` event
// to warn before full page reload and this event cannot be dispatched manually.
/* istanbul ignore next */
this._domEmitter.listenTo( window, 'beforeunload', ( evtInfo, domEvt ) => {
if ( this.isPending ) {
domEvt.returnValue = this.first.message;
}
} );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We talked with @Reinmar and agreed that this logic is not needed with no autosave plugin so we should move it there. If one does not use autosave plugins, he also does not care about closing browser with pending action. Content is not saved anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

/**
* Adds action to the list of pending actions.
*
* This method returns an action object with observable message property.
* The action object can be later used in the remove method. It also allows you to change the message.
*
* const pendingActions = editor.plugins.get( 'PendingActions' );
* const action = pendingActions.add( 'Upload in progress 0%' );
*
* action.message = 'Upload in progress 10%';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no example how to remove action.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I would move the whole sample to the plugin description.

*
* @param {String} message
* @returns {Object} Observable object that represents a pending action.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to describe properties.

*/
add( message ) {
if ( typeof message !== 'string' ) {
/**
* Message has to be a string.
*
* @error pendingactions-add-invalid-message
*/
throw new CKEditorError( 'pendingactions-add-invalid-message: Message has to be a string.' );
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 pendingactions-add-invalid-message error.

See

/**
* Cannot load a plugin because one of its dependencies is listed in the `removePlugins` option.
*
* @error plugincollection-required
* @param {Function} plugin The required plugin.
* @param {Function} requiredBy The parent plugin.
*/
throw new CKEditorError(
'plugincollection-required: Cannot load a plugin because one of its dependencies is listed in' +
'the `removePlugins` option.',
{ plugin: RequiredPluginConstructor, requiredBy: PluginConstructor }
);
.

Copy link
Contributor Author

@oskarwrobel oskarwrobel May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK there is no need to add docs to the error when an additional explanation is not necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you miss the @error JSDoc tag, the error will not appear on the error codes page:

image

After adding:

+/**
+ * Some docs for the error.
+ *
+ * @error pendingactions-add-invalid-message
+ */
throw new CKEditorError( 'pendingactions-add-invalid-message: Message has to be a string.' );

the error appears:

image

One more thing. When I built the docs, I noticed that:

No errors found during the validation.
WARNING: The @type tag does not permit a description; the description will be ignored. File: pendingactions.js, line: 55

Should it be @member instead of @type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we have more errors without docs and probably we should fix them too. I'll add docs to this error. And I'll change @type to @member :) Thx.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have more errors without docs

Would be good to verify this and report as a new ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OTOH I still think that additional explanation for this error is not necessary because the error message is clear and we don't need to explain it in the docs. @pjasiun WDYT? Should we always add docs to error or only when error needs an additional explanation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to introduce any additional description. See here – https://github.com/ckeditor/ckeditor5-engine/blob/a721f95601511a6fcbb7d43becb957af64f2d157/src/view/attributeelement.js#L109-L117

We need the proper documentation for the error in docs, so

/**
 * Message has to be a string.
 *
 * @error pendingactions-add-invalid-message
 */

is enough.

Copy link
Contributor Author

@oskarwrobel oskarwrobel May 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand. But I'm not sure if documentation for this kind of errors is really necessary. We have more undocumented errors. Let's wait for @pjasiun or @Reinmar comment. If we decided to document all errors because we want to have them in generated docs then it's fine and I'll add this doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I checked and each new type of error needs docs. When the error is repeated then docs is not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

const action = Object.create( ObservableMixin );

action.set( 'message', message );
this._actions.add( action );
this.isPending = true;

return action;
}

/**
* Removes action from the list of pending actions.
*
* @param {Object} action Action object.
*/
remove( action ) {
this._actions.remove( action );
this.isPending = !!this._actions.length;
}

/**
* Returns first action from the list.
*
* const pendingActions = editor.plugins.get( 'PendingActions' );
*
* pendingActions.add( 'Action 1' );
* pendingActions.add( 'Action 2' );
*
* pendingActions.first // Returns 'Action 1'
* Array.from( pendingActions ) // Returns [ 'Action 1', 'Action 2' ]
Copy link

@pjasiun pjasiun May 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this sample shows usage of not only this method by also iterator interface I would move it to the class description.

*
* returns {Object} Pending action object.
*/
get first() {
return this._actions.get( 0 );
}

/**
* Iterable interface.
*
* @returns {Iterable.<*>}
*/
[ Symbol.iterator ]() {
return this._actions[ Symbol.iterator ]();
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();
this._domEmitter.stopListening();
}

/**
* Fired when an action is added to the list.
*
* @event add
* @param {Object} action The added action.
*/

/**
* Fired when an action is removed from the list.
*
* @event remove
* @param {Object} action The removed action.
*/
}
11 changes: 11 additions & 0 deletions tests/manual/pendingactions.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<div>
<button id="add-action">Add pending action</button>
</div>

<h3>Pending actions list:</h3>
<ol class="pending-actions"></ol>

<div id="editor">
<h2>Heading 1</h2>
<p>Paragraph</p>
</div>
74 changes: 74 additions & 0 deletions tests/manual/pendingactions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/**
* @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/* globals console, window, document, setTimeout */

import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';

import ArticlePluginSet from '../_utils/articlepluginset';
import PendingActions from '../../src/pendingactions';

ClassicEditor
.create( document.querySelector( '#editor' ), {
plugins: [ ArticlePluginSet, PendingActions ],
toolbar: [ 'heading', '|', 'bold', 'italic', 'link', 'bulletedList', 'numberedList', 'blockQuote', 'undo', 'redo' ],
image: {
toolbar: [ 'imageStyle:full', 'imageStyle:side', '|', 'imageTextAlternative' ],
}
} )
.then( editor => {
window.editor = editor;

const pendingActions = editor.plugins.get( PendingActions );
const actionsEl = document.querySelector( '.pending-actions' );

document.querySelector( '#add-action' ).addEventListener( 'click', () => {
const action = pendingActions.add( 'Pending action 0%.' );

wait( 1000 )
.then( () => ( action.message = 'Pending action 0%.' ) )
.then( () => wait( 500 ) )
.then( () => ( action.message = 'Pending action 20%.' ) )
.then( () => wait( 500 ) )
.then( () => ( action.message = 'Pending action 40%.' ) )
.then( () => wait( 500 ) )
.then( () => ( action.message = 'Pending action 60%.' ) )
.then( () => wait( 500 ) )
.then( () => ( action.message = 'Pending action 80%.' ) )
.then( () => wait( 500 ) )
.then( () => ( action.message = 'Pending action 100%.' ) )
.then( () => wait( 500 ) )
.then( () => pendingActions.remove( action ) );
} );

pendingActions.on( 'add', () => displayActions() );
pendingActions.on( 'remove', () => displayActions() );

function displayActions() {
const frag = document.createDocumentFragment();

for ( const action of pendingActions ) {
const item = document.createElement( 'li' );

item.textContent = action.message;

action.on( 'change:message', () => {
item.textContent = action.message;
} );

frag.appendChild( item );
}

actionsEl.innerHTML = '';
actionsEl.appendChild( frag );
}

function wait( ms ) {
return new Promise( resolve => setTimeout( resolve, ms ) );
}
} )
.catch( err => {
console.error( err.stack );
} );
6 changes: 6 additions & 0 deletions tests/manual/pendingactions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
## Pending actions

1. Click `Add pending actions`
2. Check if action message is changing (action should disappear when progress reaches 100%)
3. Try to reload page or close the browser tab when pending action is displayed, you should see a prompt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not make sense now. We could have some "finish" button to check if it works.

Copy link

@pjasiun pjasiun May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you can add onbeforeunload integration to the sample.

4. Try to reload page when there are no pending actions, you shouldn't see a prompt
Loading