Feature: Introduced PendingActions plugin. #127

oskarwrobel · 2018-05-10T17:10:45Z

Suggested merge commit message (convention)

Feature: Introduced PendingActions plugin. Closes ckeditor/ckeditor5#2934.

This is a very basic implementation that covers all current needs.

It is possible to add pending action with a custom message and change this message. Message is observable.

Besides plugin listens to window#beforeunload event and displays prompt in a case of pending action.

const action = this.editor.plugins.get( 'PendingActions' ).add( 'Custom message' );

action.message = 'New message';

this.editor.plugins.get( 'PendingActions' ).remove( action );

coveralls · 2018-05-10T17:17:14Z

Coverage remained the same at 100.0% when pulling 7da48ce on t/126 into 7e7b950 on master.

pomek · 2018-05-10T17:53:27Z

src/pendingactions.js

+	 */
+	add( message ) {
+		if ( typeof message !== 'string' ) {
+			throw new CKEditorError( 'pendingactions-add-invalid-message: Message has to be a string.' );


Missing docs for the pendingactions-add-invalid-message error.

See

ckeditor5-core/src/plugincollection.js

Lines 186 to 197 in 7e7b950

/**

* 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 }

);

.

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

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

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:

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?

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.

we have more errors without docs

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

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?

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.

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.

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

pjasiun · 2018-05-11T12:32:42Z

src/pendingactions.js

+			throw new CKEditorError( 'pendingactions-add-invalid-message: Message has to be a string.' );
+		}
+
+		const observable = Object.create( ObservableMixin );


The variable should be called action, instead of observable.

pjasiun · 2018-05-11T12:51:29Z

src/pendingactions.js

+		/* istanbul ignore next */
+		this._domEmitter.listenTo( window, 'beforeunload', ( evtInfo, domEvt ) => {
+			if ( this.isPending ) {
+				domEvt.returnValue = this._actions.get( 0 ).message;


Since getting the first action looks like a common case, maybe we should have: this.get, which will do this._actions.get or the property first?

More I think about it, more I feel that the plugin should have Collection API. Not everybody feels comfortable with iterators (even you used _actions.get( 0 ), it is more convenient). So as much as I love iterator, I think that get and length should be also provided.

In f2f talk we decided to add getter first for getting first pending action from the list.

pjasiun · 2018-05-11T13:05:24Z

tests/manual/pendingactions.js

+				.then( () => wait( 500 ) )
+				.then( () => ( action.message = 'Dynamic pending action 80%.' ) )
+				.then( () => wait( 500 ) )
+				.then( () => ( action.message = 'Dynamic pending action 1000%.' ) )


pjasiun · 2018-05-11T13:17:08Z

tests/manual/pendingactions.js

+			const action = pendingActions.add( 'Static pending action.' );
+
+			wait( 5000 ).then( () => pendingActions.remove( action ) );
+		} );


Do we need these "static pending actions"? Can't we have only one type of actions in this manual tests (dynamic)? More buttons in manual tests mean more things to click, to test, more code to maintain. If "dynamic" pending actions cover all cases lets make it simple.

pjasiun · 2018-05-11T13:25:12Z

src/pendingactions.js

+	/**
+	 * Adds action to the list of pending actions.
+	 *
+	 * Methods returns an object with observable message property. Message can be changed.


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.

Also, it would be useful to put an example here.

pjasiun · 2018-05-11T13:37:35Z

src/pendingactions.js

+/**
+ * List of editor pending actions.
+ *
+ * Any asynchronous action that should be finished before the editor destroy (like file upload) should be registered


should be finished before the editor destroy

This plugin has nothing to do with editing destroy. It is to handle long-lasting actions. To synchronise plugins which execute such action (i.e. file upload) and 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 is not able to interrupt actions or manage that in any other way.

pjasiun · 2018-05-14T09:14:24Z

src/pendingactions.js

+			if ( this.isPending ) {
+				domEvt.returnValue = this.first.message;
+			}
+		} );


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.

pjasiun · 2018-05-14T16:08:32Z

src/pendingactions.js

+ * 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.


Not anymore.

pjasiun · 2018-05-14T16:10:57Z

src/pendingactions.js

+	 *		const pendingActions = editor.plugins.get( 'PendingActions' );
+	 * 		const action = pendingActions.add( 'Upload in progress 0%' );
+	 *
+	 * 		action.message = 'Upload in progress 10%';


There is no example how to remove action.

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

pjasiun · 2018-05-14T16:11:23Z

src/pendingactions.js

+	 * 		action.message = 'Upload in progress 10%';
+	 *
+	 * @param {String} message
+	 * @returns {Object} Observable object that represents a pending action.


It would be great to describe properties.

pjasiun · 2018-05-14T16:15:56Z

src/pendingactions.js

+	 * 		pendingActions.add( 'Action 2' );
+	 *
+	 *		pendingActions.first // Returns 'Action 1'
+	 *		Array.from( pendingActions ) // Returns [ 'Action 1', 'Action 2' ]


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

pjasiun · 2018-05-15T14:21:48Z

tests/manual/pendingactions.md

+
+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


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

Or you can add onbeforeunload integration to the sample.

oskarwrobel added 6 commits May 10, 2018 13:18

Feature: Introduced PendingActions plugin.

2b70742

Added browser prompt on beforeunload event in a case of pending actions.

09bc3b5

Test: Added manual test for PendingActions plugin.

9e45700

Added add and remove events to PendingActions plugin.

d1e822f

Used relative paths.

ab9a190

Docs: Minor changes.

dcb6391

oskarwrobel assigned pjasiun May 10, 2018

Docs: Add plugin general docs.

038c606

pomek reviewed May 10, 2018

View reviewed changes

oskarwrobel mentioned this pull request May 10, 2018

Feature: Added upload pending action ckeditor/ckeditor5-upload#85

Merged

oskarwrobel force-pushed the t/126 branch from 038c606 to 08c4731 Compare May 11, 2018 07:06

pjasiun reviewed May 11, 2018

View reviewed changes

pjasiun added the status:review- label May 11, 2018

oskarwrobel added 5 commits May 11, 2018 16:24

Docs: Added missing error docs.

1858961

Renamed observable to action.

e322977

Added first getter to the PendingActions plugin.

6888e2e

Tests: Simplified PendingActions manual test.

b557dca

Docs: Imporved PendingActions docs.

ef85a34

oskarwrobel force-pushed the t/126 branch from 08c4731 to ef85a34 Compare May 11, 2018 15:07

oskarwrobel removed the status:review- label May 11, 2018

pjasiun reviewed May 14, 2018

View reviewed changes

Removed checking pending actions on beforeunload event.

387e09e

pjasiun reviewed May 14, 2018

View reviewed changes

oskarwrobel added 2 commits May 15, 2018 09:23

Docs: Improved snippets.

15e8157

Improved code style.

5c7b465

pjasiun reviewed May 15, 2018

View reviewed changes

oskarwrobel added 3 commits May 15, 2018 16:27

Docs: Fixed indents.

e127008

Tests: Added onbeforeunload handlet to PandingActions manual test.

ece700c

Docs: Removed unnecessary empty line from comment.

7da48ce

pjasiun merged commit e1af648 into master May 15, 2018

pjasiun deleted the t/126 branch May 15, 2018 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: Introduced PendingActions plugin. #127

Feature: Introduced PendingActions plugin. #127

oskarwrobel commented May 10, 2018 •

edited by pomek

Loading

coveralls commented May 10, 2018 •

edited

Loading

pomek May 10, 2018

oskarwrobel May 10, 2018 •

edited

Loading

pomek May 11, 2018

oskarwrobel May 11, 2018

pomek May 11, 2018

oskarwrobel May 11, 2018

pomek May 11, 2018

oskarwrobel May 11, 2018 •

edited

Loading

oskarwrobel May 11, 2018

oskarwrobel May 11, 2018

pjasiun May 11, 2018

oskarwrobel May 11, 2018

pjasiun May 11, 2018 •

edited

Loading

pjasiun May 11, 2018

oskarwrobel May 11, 2018

pjasiun May 11, 2018 •

edited

Loading

oskarwrobel May 11, 2018

pjasiun May 11, 2018 •

edited

Loading

oskarwrobel May 11, 2018

pjasiun May 11, 2018 •

edited

Loading

pjasiun May 11, 2018

pjasiun May 11, 2018

pjasiun May 14, 2018

oskarwrobel May 14, 2018

pjasiun May 14, 2018

pjasiun May 14, 2018

pjasiun May 14, 2018

pjasiun May 14, 2018

pjasiun May 14, 2018 •

edited

Loading

pjasiun May 15, 2018

pjasiun May 15, 2018 •

edited

Loading

	/**
	* 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 }
	);

Feature: Introduced PendingActions plugin. #127

Feature: Introduced PendingActions plugin. #127

Conversation

oskarwrobel commented May 10, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

coveralls commented May 10, 2018 • edited Loading

Choose a reason for hiding this comment

oskarwrobel May 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarwrobel May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun May 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjasiun May 15, 2018 • edited Loading

Choose a reason for hiding this comment

oskarwrobel commented May 10, 2018 •

edited by pomek

Loading

coveralls commented May 10, 2018 •

edited

Loading

oskarwrobel May 10, 2018 •

edited

Loading

oskarwrobel May 11, 2018 •

edited

Loading

pjasiun May 11, 2018 •

edited

Loading

pjasiun May 11, 2018 •

edited

Loading

pjasiun May 11, 2018 •

edited

Loading

pjasiun May 11, 2018 •

edited

Loading

pjasiun May 14, 2018 •

edited

Loading

pjasiun May 15, 2018 •

edited

Loading