Introduced mentions plugin #1932

jacekbogdanski · 2018-04-27T12:54:52Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

This PR continues work on #1703 plugin. See #1757 for more details.

Closes #1703, #2101

mlewand

So far just a couple of docs related issues.

Most importantly we need better docs for config.mentions option. There should be an abstract type like CKEDITOR.plugins.mentions.configDefinition that would document config option.

Then config.mentions should be set as a CKEDITOR.plugins.mentions.configDefinition[] type. Also config parameter of CKEDITOR.plugin.mentions type should be marked as CKEDITOR.plugins.mentions.configDefinition.

mlewand · 2018-05-09T09:01:37Z

plugins/mentions/plugin.js

+'use strict';
+
+( function() {
+	var MARKER = '@', MIN_CHARS = 2, ENCODED_QUERY = '{encodedQuery}';


Do not define multiple vars in the same line.

mlewand · 2018-05-09T09:36:17Z

plugins/mentions/plugin.js

+		this.minChars = config.minChars !== null && config.minChars !== undefined ? config.minChars : MIN_CHARS;
+
+		/**
+		 * {@link CKEDITOR.plugins.autocomplete Autocomplete} instance used by mentions feature to implement autocompletion logic.


This should be a private property.

mlewand · 2018-05-09T09:37:06Z

plugins/mentions/plugin.js

+	 * @since 4.10.0
+	 * @constructor Creates the new instance of mentions and attaches it to the editor using {@link CKEDITOR.plugins.autocomplete Autocomplete feature}.
+	 * @param {CKEDITOR.editor} editor The editor to watch.
+	 * @param {Object} config Configuration object keeping information how to instantiate mentions plugin.


This parameter should use abstract type definition, for an example, it could be named CKEDITOR.plugins.mentions.configDefinition.

mlewand · 2018-05-09T09:42:29Z

plugins/mentions/plugin.js

+		 * A character that should trigger autocompletion.
+		 * @property {String} [marker='@']
+		 */
+		this.marker = config.marker || MARKER;


This property is not meant to be modified. Thus it should be marked as readonly.

mlewand · 2018-05-09T09:42:53Z

plugins/mentions/plugin.js

+		 * A number of characters that should follow the marker character in order to trigger mentions feature.
+		 * @property {Number} [minChars=2]
+		 */
+		this.minChars = config.minChars !== null && config.minChars !== undefined ? config.minChars : MIN_CHARS;


Same as this.marker (readonly).

mlewand · 2018-05-09T09:45:42Z

plugins/mentions/plugin.js

+
+		/**
+		 * A character that should trigger autocompletion.
+		 * @property {String} [marker='@']


Please, leave consistently one line between property/method description and API doc tags section. This issue appears in other places too.

mlewand · 2018-05-09T09:47:12Z

plugins/mentions/plugin.js

+		this.marker = config.marker || MARKER;
+
+		/**
+		 * A number of characters that should follow the marker character in order to trigger mentions feature.


"marker character" should be linked, so for instance {@link #marker marker character}.

mlewand · 2018-05-09T09:49:03Z

plugins/mentions/plugin.js

+	 *
+	 * @class CKEDITOR.plugins.mentions
+	 * @since 4.10.0
+	 * @constructor Creates the new instance of mentions and attaches it to the editor using {@link CKEDITOR.plugins.autocomplete Autocomplete feature}.


"using Autocomplete feature" is not needed here. It should be simply "to the editor.", period. The fact that it utilizes autocomplete under the hood is not important for docs reader at this level.

mlewand · 2018-05-09T09:51:43Z

plugins/mentions/plugin.js

+			getTextTestCallback( this.marker, this.minChars ),
+			getDataCallback( feed, this.marker, this.caseSensitive ) );
+
+		template && this.changeViewTemplate( template );


Do not use x && y() form. Use a regular ifs.

mlewand · 2018-05-09T09:53:24Z

plugins/mentions/plugin.js

+		},
+
+		/**
+		 * Changes the view template. The given template will be used by {@link CKEDITOR.template}.


The given template will be used by {@link CKEDITOR.template}.

I don't think this has value. Focus on the purpose of method, rather on how something is done.

mlewand · 2018-05-11T15:10:18Z

During the review I noticed a major requirement that is needed before deploying the feature:

Mentions plugin cache #1969 - response caching

mlewand

I have extracted issue related to caching to a separate issue so that it's easier to track.

There's also an issue with possible race condition. Also I noted multiple things related to docs. I want to stress that the docs are vital for this feature. Also it's about time we start to work on sdk sample and docs in paraler to this feature (ckeditor/ckeditor4-sdk#240).

The proposed solution is prone to response race conditions.

A scenario where server responses does not come in exact order, as they were issued, is pretty possible - wile easy to avoid. We should consider that too.

Imagine that the user opens CKE with "@ann" as a content and wants to type "@annaB" in order to use "Annabelle" suggestion.

User starts typing, mentions plugin detects "@anna".
Server at this point sends "Anna", "Annabelle" response but client did not get it yet! Let's call it response A.
User presses "b" so there's "@annaB" (this is few ms after the previous step).
Server at this point sends "Annabelle" response but client did not get it yet! Let's call it response B.
User receives response B (note this server response is out of order!).
User receives response A.

Expected

Mention plugin shows only "Annabelle" option.

Actual

Mention plugin shows both "Anna" and "Annabelle" options.

mlewand · 2018-05-09T15:46:57Z

plugins/mentions/plugin.js

+	/**
+	 * A character that should trigger autocompletion.
+	 *
+	 * @property {String} [marker='@'] marker


This is not a valid @property tag syntax. What's more is that it leaks artefacts to the rendered:

mlewand · 2018-05-10T12:00:39Z

plugins/mentions/plugin.js

+	} );
+
+	/**
+	 * Mentions plugin allows you to type custom marker character and get suggested values for the usernames so that you don't have to write it on your own.


for the usernames so that

This functionallity is not limited to user names. It could reference just anything.

mlewand · 2018-05-10T12:03:51Z

plugins/mentions/plugin.js

+	 * Mentions plugin allows you to type custom marker character and get suggested values for the usernames so that you don't have to write it on your own.
+	 *
+	 * The recommended way to add mentions feature to an editor is by
+	 * using {@link CKEDITOR.config#mentions} configuration property or by passing it as the configuration option when instantiating an editor.


I think a better inner text would be config.mentions also "configuration" word is redundant.

See how it looks currently:

(…) using CKEDITOR.config.mentions configuration property (…)

What I'm proposing is something more like

(…) using config.mentions property (…)

mlewand · 2018-05-10T12:09:59Z

plugins/mentions/plugin.js

+	 *
+	 * ```javascript
+	 * // Simple usage with CKEDITOR.config.mentions property.
+	 * CKEDITOR.config.mentions = [ { feed: ['Anna', 'Thomas', 'John'], minChars: 0, marker: '#' } ];


Let's skip marker property customization here. minChars is fine.

Also this is not an optimal way to change this configuration.

This code will most likely not work when the plugin is loaded dynamically.

Imagine you have website code like:

<script src="ckeditor/4.9.2/ckeditor.js"></script> <script> CKEDITOR.config.mentions = [ { feed: ['Anna', 'Thomas', 'John'], minChars: 0, marker: '#' } ]; CKEDITOR.replace( 'editable', { extraPlugins: mentions } ); </script>

Your config will get overridden. Why is that? Because plugin.js will get loaded asynchronously. Meaning that your default value will kick in after the value above was assigned.

Please update other places accordingly. I think in this case you need to refer to config.mentions = [ (…) ]. In case of doubts contact our docs manager.

In that case shouldn't we remove default CKEDITOR.config.mentions = [] initialization so it won't overwrite user configuration?

Yup, that's fine.

mlewand · 2018-05-10T12:10:08Z

plugins/mentions/plugin.js

+	 *
+	 * // Passing mentions configuration when creating editor.
+	 * CKEDITOR.replace( 'editor', {
+	 * 		mentions: [ { feed: ['Anna', 'Thomas', 'John'], minChars: 0, marker: '#' } ]


Same as above:

Let's skip marker property customization here. minChars is fine.

mlewand · 2018-05-11T15:24:22Z

plugins/mentions/plugin.js

+
+			callback = getDataCallbackWithMarkedItems( marker, callback );
+
+			// Feed is a simple array e.g. [ 'Anna', 'John', 'Thomas' ].


Blocks:

// Feed is a simple array e.g. [ 'Anna', 'John', 'Thomas' ].

// Feed is a URL to be requested for a JSON of matches e.g. /user-controller/get-list/{encodedQuery}.

// Feed is a function which uses callback to pass data through autocomplete plugin e.g.

Should be palced in feed property API docs rather than here in the code.

mlewand · 2018-05-11T15:28:06Z

plugins/mentions/plugin.js

+	 */
+
+	/**
+	 * Feed of items to be displayed in mentions plugin.


This property is not properly documented. It's extremely important property, while this its entire doc:

You have placed a lot of it in mentions.configDefinition type description. I feel that type definition should be very short and simple.

It make sense to add one listingbut that's about it. From there it should reference feed property for further reading. I think it will be much better path.

mlewand · 2018-05-11T15:29:31Z

plugins/mentions/plugin.js

+	/**
+	 * Template used to render matches in the dropdown.
+	 *
+	 * @property {String} [template=CKEDITOR.plugins.autocomplete.view.itemTemplate] template


What attributes does it (the template) take? At least code listing would be valuable here.

mlewand · 2018-05-11T17:03:31Z

plugins/mentions/plugin.js

+				var encodedQueryRegex = new RegExp( ENCODED_QUERY, 'g' ),
+					encodedUrl = feed.replace( encodedQueryRegex, encodeURIComponent( query ) );
+
+				CKEDITOR.ajax.load( encodedUrl, function( data ) {


This code doesn't handle response race conditions. I'll inlcude the full repro case in review summary.

mlewand · 2018-05-11T17:04:34Z

plugins/mentions/plugin.js

+
+			// Feed is a URL to be requested for a JSON of matches e.g. `/user-controller/get-list/{encodedQuery}`.
+			else if ( typeof feed === 'string' ) {
+				var encodedQueryRegex = new RegExp( ENCODED_QUERY, 'g' ),


Please reuse CKEDITOR.template type for replacement logic.

jacekbogdanski · 2018-05-14T13:15:30Z

The issue #1932 (review) has been handled inside autocomplete plugin. Although I will create some additional tests to be sure it won't be accidentally broken by future autocomplete plugin changes.

jacekbogdanski · 2018-05-25T11:13:06Z

Rebased into t/1751.

jacekbogdanski · 2018-05-30T12:39:21Z

Rebased into t/1751.

jacekbogdanski · 2018-06-19T10:00:59Z

I rebased this PR into the latest upstream. I also referenced #2101 issue.

This reverts commit 2451cf6.

… not take an effect.

Added mentions items limit option

…f it consisted of more than 1 character. Also simplified unit tests a little.

mlewand

Tests are running and failing on IE8.

mlewand · 2018-06-20T08:12:33Z

tests/plugins/mentions/manual/cache.md

+
+1. Focus the editor.
+1. Type `@anna`.
+1. Press enter.


It would be nice to add a point to type a space, because without space, if you start to type @anna@anna or anything else, text the text will not issue the request anyway.

mlewand · 2018-06-20T08:15:49Z

tests/plugins/mentions/manual/cache.md

+
+## Expected
+
+History log doesn't contain duplicated URL requests.


URL requests - we should simplify it, as the tester does not see URLs but simply something like:

Request: "" Request: "a" Request: "an" Request: "anna" Request: "annaJ" Request: "j" Request: "joh" Request: "john"

Let's refer to it just as request.

mlewand · 2018-06-20T09:01:31Z

tests/plugins/mentions/manual/mentions.md

@@ -0,0 +1,10 @@
+@bender-tags: mentions, feature, 4.10.0, 1703


This manual test deserves to become a generic manual test (so that it gets tested during each future release).

To do that you need to remove feature feature/bug branch.

mlewand · 2018-06-20T09:16:51Z

plugins/mentions/plugin.js

+	 *
+	 * ```javascript
+	 * // Simple usage with CKEDITOR.config.mentions property.
+	 * CKEDITOR.config.mentions = [ { feed: ['Anna', 'Thomas', 'John'], minChars: 0 } ];


Recommended way is to set it up by passing the config directly to the editor instance, global config, although valid, is not a common method in CKE4. So I'd switch these two.

mlewand · 2018-06-20T09:23:14Z

plugins/mentions/plugin.js

+		return function( query, range, callback ) {
+			// We are removing marker here to give clean query result for the endpoint callback.
+			if ( mentions.marker ) {
+				query = query.substring( 1 );


The length of a marker should be dynamically computed, so mentions.marker.length instead of magic number 1

mlewand · 2018-06-20T09:56:53Z

I have pushed a commit fixing most of the above issues, except IE8 thing. I don't see more issues than that so please correct it and we're good to go! 👍

mlewand

Great, looks good 👌

mlewand self-requested a review May 9, 2018 09:01

mlewand reviewed May 9, 2018

View reviewed changes

mlewand suggested changes May 11, 2018

View reviewed changes

jacekbogdanski requested a review from mlewand May 15, 2018 12:59

jacekbogdanski mentioned this pull request May 24, 2018

Added mentions output template #1992

Merged

2 tasks

jacekbogdanski force-pushed the t/1751 branch from ab712c3 to ac6bad8 Compare May 24, 2018 16:42

jacekbogdanski force-pushed the t/1703-b branch from 81e5cbd to 81598c2 Compare May 25, 2018 11:11

jacekbogdanski force-pushed the t/1703-b branch from d3f2e94 to e221b31 Compare May 30, 2018 12:37

jacekbogdanski force-pushed the t/1703-b branch 2 times, most recently from 7f26536 to d143232 Compare June 11, 2018 08:27

jacekbogdanski force-pushed the t/1703-b branch 2 times, most recently from e8c25fa to b457315 Compare June 19, 2018 09:56

jacekbogdanski and others added 13 commits June 19, 2018 14:26

Revert "Removed mentions plugin."

144f9e2

This reverts commit 2451cf6.

Added caseSensitive option.

8c2d41f

Added unit tests.

8b7d152

Updated docs.

cf7aeee

Tests: added manual tests and issue tags.

07bbd56

Tests: added extra TC for matching sensitivity.

af22314

Docs: placed some more emphasize on when case sensitivity option does…

32bb40d

… not take an effect.

Updated docs.

0a7cea2

Reworded docs and added mentions configuration definition.

1de3cf0

Refactoring.

d2e9f6a

Updated docs.

da2593c

Removed CKEDITOR.config.mentions pre initialization.

840a318

Refactored URL feed.

f7d7e0d

jacekbogdanski and others added 13 commits June 19, 2018 14:26

Added mentions view template manual tests.

ac8437a

Reworded docs.

ffaac9a

Review corrections.

3ffb4bd

Fixed small issue with invalid text matching.

63c7aa6

Simplified the mentions dev sample content.

86c53ed

Allow headers in mentions plugin dev sample.

333670f

Rebase fixes.

4d8c4c2

Added throttling.

349b483

Typo fix.

41cd52f

Docs: removed one extra step to get to the doc of given property.

38eb64e

Rebase fixes.

d21bc23

Inherited docs.

f99d4d8

Changelog fixes.

305bde6

jacekbogdanski force-pushed the t/1703-b branch from b457315 to 305bde6 Compare June 19, 2018 12:28

jacekbogdanski changed the base branch from t/1751 to major June 19, 2018 12:29

jacekbogdanski and others added 8 commits June 19, 2018 14:33

Added items limit option.

287547b

Merge pull request #2103 from ckeditor/t/2100

bc25c71

Added mentions items limit option

Fixed test on IE10.

5f00344

Adjusted manual test.

44b46df

Corrections in Mentions manual tests.

909305a

Docs: adjusted Mentions API docs.

b7a80d4

Fixed a bug where Mentions would not include part of marker in case i…

1a6cada

…f it consisted of more than 1 character. Also simplified unit tests a little.

Tests: minor unit tests adjustments.

61d99f7

mlewand suggested changes Jun 20, 2018

View reviewed changes

jacekbogdanski requested a review from mlewand June 20, 2018 10:26

Ignored ie8 tests.

906868f

mlewand approved these changes Jun 20, 2018

View reviewed changes

mlewand merged commit 7e42f91 into major Jun 20, 2018

jacekbogdanski mentioned this pull request Jun 22, 2018

Remove docs duplication between mentions and autocomplete #2101

Closed


		callback = getDataCallbackWithMarkedItems( marker, callback );

		// Feed is a simple array e.g. [ 'Anna', 'John', 'Thomas' ].


		## Expected

		History log doesn't contain duplicated URL requests.

		@@ -0,0 +1,10 @@
		@bender-tags: mentions, feature, 4.10.0, 1703

Introduced mentions plugin #1932

Introduced mentions plugin #1932

Conversation

jacekbogdanski commented Apr 27, 2018 • edited Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

mlewand left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand commented May 11, 2018

mlewand left a comment

Choose a reason for hiding this comment

Expected

Actual

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

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

jacekbogdanski commented May 14, 2018

jacekbogdanski commented May 25, 2018

jacekbogdanski commented May 30, 2018

jacekbogdanski commented Jun 19, 2018

mlewand left a 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

mlewand commented Jun 20, 2018 • edited Loading

mlewand left a comment

Choose a reason for hiding this comment

jacekbogdanski commented Apr 27, 2018 •

edited

Loading

mlewand commented Jun 20, 2018 •

edited

Loading