Telephone link type support in link plugin #2214

engineering-this · 2018-07-11T09:40:44Z

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?

Added new dialog input[type="tel"]. And used in in link plugin for inputing phone number in new link type. Also added config options to set validation regex and alert message when validation fails.

Closes #2154

Comandeer

Field for telephone number is marked as required yet it is allowed to have empty value. IMO even without validation editor should disallow inserting empty telephone number.

Additionally there is no unit test for inserting telephone link without validation.

Comandeer · 2018-07-12T15:24:15Z

plugins/link/dialogs/link.js

+		var editor = this.getDialog()._.editor,
+			regExp =  editor.config.linkTelNumberValidate_regExp,
+			msg = editor.config.linkTelNumberValidate_msg,
+			dialog = this.getDialog();


If we move this variable definition earlier, we could use it to get editor:

editor = dialog._.editor

Microoptimisations – FTW!

Comandeer · 2018-07-12T15:27:59Z

tests/plugins/link/link.js

+			bot.dialog( 'link', function( dialog ) {
+				var expectedNumber = '123456789',
+					input,
+					stub = this._stubs.alert = sinon.stub( window, 'alert' );


TBH creating this._stubs and whole tearDown logic seems a little bit like overkill. We got only this one stub, which could be restored with one line of code instead of current 7. Keeping whole stubbing code inside test itself also improves readability.

Comandeer

Editors in manual tests are not entirely visible:

Comandeer · 2018-07-16T13:57:47Z

tests/plugins/link/link.js

+		},
+
+		// (#2154)
+		'test telephone number link without validation': assertTelephoneLinks(),


If test is moved into dedicated function, then this function should be customisable. The bare minimum is the ability to pass object with input and expected output. Otherwise the dedicated function is not reusable in future tests that could be added later, in another issue.

Also asserts could be moved into callback.

Comandeer · 2018-07-16T14:23:45Z

plugins/link/dialogs/link.js

+			regExp =  editor.config.linkTelNumberValidate_regExp,
+			msg = editor.config.linkTelNumberValidate_msg,
+			linkLang = editor.lang.link,
+			func = CKEDITOR.dialog.validate.notEmpty( linkLang.noTel ),


func could be just inlined into messageWhenEmpty variable.

Comandeer · 2018-07-16T14:23:53Z

plugins/link/dialogs/link.js

+			msg = editor.config.linkTelNumberValidate_msg,
+			linkLang = editor.lang.link,
+			func = CKEDITOR.dialog.validate.notEmpty( linkLang.noTel ),
+			messageWhenEmpty = func.apply( this );;


Double semicolon.

Comandeer · 2018-07-16T14:24:59Z

plugins/link/dialogs/link.js

+			return true;
+		}
+
+		if ( typeof messageWhenEmpty === 'string' || !messageWhenEmpty ) {


Can't it be simplified to message !== true?

Comandeer · 2018-07-16T14:43:25Z

plugins/link/dialogs/link.js

+			return messageWhenEmpty;
+		}
+
+		if ( regExp && !regExp.test( this.getValue() ) ) {


Note that we have built-in CKEDITOR.dialog.validate.regex, which could simplify the logic.

Comandeer · 2018-07-16T14:46:46Z

plugins/link/plugin.js

+		 *		config.linkTelNumberValidate_regExp = /^[0-9]{9}$/;
+		 *
+		 * @since 4.11.0
+		 * @cfg {RegExp} [linkTelNumberValidate_regExp]


I'm thinking about changing names of variables to linkTelNumberRegExp and linkTelNumberMsg. ATM only these two config variables in link plugin contain underscore, introducing some kind of inconsistency.

Comandeer

LGTM! I've just updated manual tests, rephrase message about empty input and fixed some code style issues.

mlewand · 2018-07-20T21:56:29Z

@engineering-this @Comandeer I don't think we made a good choice with the config.linkTelNumberMsg:

The message about invalid phone format should be simply put in the lang files and provided by our plugin. By putting it in the config you put the hassle of localizing this message to the dev integrating CKE4, while it should just work™. Providing it in lang file provides localization, contributed by our amazing community out of the box, for everyone.
Secondly I think the name does not fully explains the purpose. Name is linkTelNumberMsg and it does n't mention that it's a message in case of invalid number/wrong format.

Comandeer · 2018-07-20T22:06:07Z

The reason behind putting it in config was the fact that validation will be custom – so the message also could be custom, to include information about the desired format of the telephone number.

mlewand · 2018-07-20T22:50:49Z

@Comandeer That sounds like a good use case to remember about, though you can still define the default message in the lang file and allow for customization in case of need.

So the default lang entry would look pretty much like that:

en
invalidTelNumberMessage: 'Invalid phone number format.',

While you could optionally override it. It could be with config, so you would have a config.linkInvalidTelNumberMessage = 'Format should follow an XYZ pattern.'.

I have emphasized could because I'm not a big fan of adding a config option to customize the error message alone, instead I'd like to see some reusable way to adjust any lang entry during the runtime.

TBH I don't know any clear right off the bat (would need to check the code), but you can always do that by listening to instanceReady event and overwriting any lang property that should be customized. However I know, it's pretty advanced as for such use case.

engineering-this added 13 commits July 10, 2018 12:50

Added telephone number input

9b2d8c5

Added support for links with telephone numbers

b80e3c0

Added stylels for .cke_dialog_ui_input_tel

745b4a6

Added telephone entries to en langfiles

f7a39e9

Generated lang files

4c9c50e

Added test case

4061ffc

Added tel numbers optional validation

a6e875b

Added manual test

839ec1c

Phone validation now triggers only on phone links

6c07655

Added changelog entry

7394b4b

Added manual tests for styles

ed036d8

Added assertion for link validation

bece1c4

Renamed telephone numbers validation config entry and added docs entry

b73d369

engineering-this changed the title ~~Telephone linke type support in link plugin~~ Telephone link type support in link plugin Jul 11, 2018

mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Jul 11, 2018

Comandeer requested changes Jul 12, 2018

View reviewed changes

engineering-this added 6 commits July 14, 2018 10:49

Added test case without link validation

63ecb20

Minor refactor

da3b499

Added validation of empty links

da426fd

Added message for empty link telephone number

56c3be9

Changed priority of returned messages in tel link validation

15e8889

Testing validation of empty telephone links

57189a1

engineering-this requested a review from Comandeer July 14, 2018 09:26

Comandeer requested changes Jul 16, 2018

View reviewed changes

engineering-this added 4 commits July 17, 2018 11:22

Set bender ui to collapsed

889daf0

ValidateTelNumber function refactoring

ae56d56

Renamed linkTel validation config options

ff4269c

Refactored tel link tests

2a79ede

engineering-this requested a review from Comandeer July 17, 2018 12:11

Update manual test.

ebdf8c4

Comandeer added 3 commits July 17, 2018 19:40

Update message for empty telephone number.

1fe2b57

Update language files.

c305287

Fix code style issues.

a7d920b

Comandeer approved these changes Jul 17, 2018

View reviewed changes

Comandeer merged commit 1545a0e into major Jul 17, 2018

Comandeer deleted the t/2154 branch July 17, 2018 18:01

jeremy-farrance mentioned this pull request May 28, 2021

DNN Connect CKEditor Provider: Link Dialogue Window displays orphaned Telephone Number textbox dnnsoftware/Dnn.Platform#4689

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Telephone link type support in link plugin #2214

Telephone link type support in link plugin #2214

engineering-this commented Jul 11, 2018

Comandeer left a comment

Comandeer Jul 12, 2018

Comandeer Jul 12, 2018

Comandeer left a comment

Comandeer Jul 16, 2018

Comandeer Jul 16, 2018

Comandeer Jul 16, 2018

Comandeer Jul 16, 2018

Comandeer Jul 16, 2018

Comandeer Jul 16, 2018

Comandeer left a comment

mlewand commented Jul 20, 2018

Comandeer commented Jul 20, 2018

mlewand commented Jul 20, 2018

Telephone link type support in link plugin #2214

Telephone link type support in link plugin #2214

Conversation

engineering-this commented Jul 11, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer 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

Comandeer left a comment

Choose a reason for hiding this comment

mlewand commented Jul 20, 2018

Comandeer commented Jul 20, 2018

mlewand commented Jul 20, 2018