Add Ctrl+K keystroke for link command #2479

Comandeer · 2018-10-15T09:28:48Z

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?

I've added new keystroke for link command. Additionally I added tests to check if it's working alongside the old Ctrl + L one.

Closes #2478.

jacekbogdanski

I'm wondering if we could deprecate CTRL + L keystroke. TBO I hate this keystroke thus it conflicts with default browser keybindings. However, not sure if we should break old user habits. WDYT?

jacekbogdanski · 2018-10-15T09:46:26Z

CHANGES.md

@@ -12,6 +12,7 @@ New Features:
 * [#706](https://github.com/ckeditor/ckeditor-dev/issues/706): Added different cursor style when selecting cells for [Table Selection](https://ckeditor.com/cke4/addon/tableselection) plugin.
 * [#651](https://github.com/ckeditor/ckeditor-dev/issues/651): Text pasted using [Paste from Word](https://ckeditor.com/cke4/addon/pastefromword) preservers indentation in paragraphs.
 * [#1176](https://github.com/ckeditor/ckeditor-dev/pull/1176): The [Balloon Panel](https://ckeditor.com/cke4/addon/balloonpanel) can be attached to selection instead of element.
+* [#2478](https://github.com/ckeditor/ckeditor-dev/issues/2478): Link could be inserted using <kbd>Ctrl</kbd>/<kbd>Cmd</kbd> + <kbd>K</kbd> keystroke.


[Link](https://ckeditor.com/cke4/addon/link) can be inserted using <kbd>Ctrl</kbd>/<kbd>Cmd</kbd> + <kbd>K</kbd> keystroke.

jacekbogdanski · 2018-10-15T09:48:34Z

tests/plugins/link/manual/keystroke.md

+
+Link dialog is open.
+
+Repeat the procedure for <kbd>Ctrl</kbd>/<kbd>Cmd</kbd> + <kbd>L</kbd> keystroke.


I would merge it with reproduction steps, so it will be easier to track expected/unexpected.

jacekbogdanski · 2018-10-15T09:50:33Z

tests/plugins/link/manual/keystroke.md

+1. Focus the editor.
+2. Press <kbd>Ctrl</kbd>/<kbd>Cmd</kbd> + <kbd>K</kbd>.
+
+## Expected


I'm missing ## Unexpected section. I know it may seem redundant but let's keep it consistent within a project.

jacekbogdanski · 2018-10-15T10:44:30Z

tests/plugins/link/link.js

+			} );
+		},
+
+		// (#2478)


This case is not a part of the issue so I would remove tag reference here.

I'd keep the reference and treat this one as prevention from regression (accidentally breaking the former keystroke).

I'd second to what @jacekbogdanski noted, this case is not related. Regression protection (related to #2478) is provided by the test Ctrl+K keystroke test case.

jacekbogdanski · 2018-10-15T10:48:39Z

tests/plugins/link/link.js

+					resume( function() {
+						var dialog = evt.data;
+
+						dialog.hide();


Instead of hiding dialog here, you could move the whole teardown login into tearDown test function. Something like:

var dialog = CKEDITOR.dialog.getCurrent(); if ( dialog ) { dialog.hide(); }

It also applies to the second unit test.

I'm not sure if it's necessary. Every other test in this file hides dialog on its own. Having two tests that are using tearDown for it would break consistency.

#2479 (comment)

Not if you refactor other tests. However, following the rule "If it ain't broke, don't fix it", lets deal with important stuff, not test refactoring.

mlewand · 2018-10-15T10:54:44Z

I'm not sure about removing the hotkey, we'd need to have some data on how people really use the app to back it up.

E.g. personally I find ctrl + l much more intuitive. I also do not miss the native hotkey, as I use alt + d for focusing the address bar (more convenient to press).

Comandeer · 2018-10-15T11:08:59Z

I'm also against removing the old keystroke. It was present from the very beginning and probably many people are accustomed to it.

jacekbogdanski

LGTM

jacekbogdanski · 2018-10-15T11:44:39Z

tests/plugins/link/manual/keystroke.md

+1. Focus the editor.
+2. Press <kbd>Ctrl</kbd>/<kbd>Cmd</kbd> + <kbd>K</kbd>.
+
+	## Expected


I'm not really sure about test construction. I'm more used to linear tests. Although it's subjective, so would like to see the opinion of another reviewer.

jacekbogdanski · 2018-10-15T11:49:32Z

tests/plugins/link/link.js

+					resume( function() {
+						var dialog = evt.data;
+
+						dialog.hide();


#2479 (comment)

Not if you refactor other tests. However, following the rule "If it ain't broke, don't fix it", lets deal with important stuff, not test refactoring.

mlewand

Looks good, the only thing that bugs me is this ticket reference.

There's also a copy'n'pasta in unit tests, but we can let it go as long as there's no third use.

mlewand

Looks ok, I have rebased the branch.

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

mlewand requested a review from jacekbogdanski October 15, 2018 09:30

jacekbogdanski self-assigned this Oct 15, 2018

jacekbogdanski requested changes Oct 15, 2018

View reviewed changes

CKEditorBot unassigned jacekbogdanski Oct 15, 2018

Comandeer requested a review from jacekbogdanski October 15, 2018 11:17

jacekbogdanski self-assigned this Oct 15, 2018

jacekbogdanski approved these changes Oct 15, 2018

View reviewed changes

CKEditorBot unassigned jacekbogdanski Oct 15, 2018

jacekbogdanski requested a review from mlewand October 15, 2018 11:54

mlewand self-assigned this Oct 15, 2018

mlewand reviewed Oct 15, 2018

View reviewed changes

Comandeer requested a review from mlewand October 15, 2018 12:32

Comandeer added 8 commits October 15, 2018 14:43

Add unit tests.

863c9ce

Add manual test.

d7734e5

Add changelog entry.

97ba263

Add new keystroke.

0088771

Update changelog entry.

37ef5d2

Update manual test.

206257a

Remove issue referecne.

548aac6

Move logic for keystroke tests into separate helper function.

56ab704

mlewand force-pushed the t/2478 branch from 7190c49 to 56ab704 Compare October 15, 2018 12:43

mlewand approved these changes Oct 15, 2018

View reviewed changes

CKEditorBot unassigned mlewand Oct 15, 2018

mlewand merged commit e413fab into major Oct 15, 2018

CKEditorBot deleted the t/2478 branch October 15, 2018 12:44

mlewand added this to the 4.11.0 milestone Oct 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Ctrl+K keystroke for link command #2479

Add Ctrl+K keystroke for link command #2479

Comandeer commented Oct 15, 2018

jacekbogdanski left a comment

jacekbogdanski Oct 15, 2018

jacekbogdanski Oct 15, 2018

jacekbogdanski Oct 15, 2018

jacekbogdanski Oct 15, 2018

Comandeer Oct 15, 2018

mlewand Oct 15, 2018

jacekbogdanski Oct 15, 2018

Comandeer Oct 15, 2018

jacekbogdanski Oct 15, 2018 •

edited

Loading

mlewand commented Oct 15, 2018

Comandeer commented Oct 15, 2018

jacekbogdanski left a comment

jacekbogdanski Oct 15, 2018

jacekbogdanski Oct 15, 2018 •

edited

Loading

mlewand left a comment

mlewand left a comment


		Link dialog is open.

		Repeat the procedure for <kbd>Ctrl</kbd>/<kbd>Cmd</kbd> + <kbd>L</kbd> keystroke.

Add Ctrl+K keystroke for link command #2479

Add Ctrl+K keystroke for link command #2479

Conversation

Comandeer commented Oct 15, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

jacekbogdanski 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

jacekbogdanski Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

mlewand commented Oct 15, 2018

Comandeer commented Oct 15, 2018

jacekbogdanski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacekbogdanski Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

mlewand left a comment

Choose a reason for hiding this comment

mlewand left a comment

Choose a reason for hiding this comment

jacekbogdanski Oct 15, 2018 •

edited

Loading

jacekbogdanski Oct 15, 2018 •

edited

Loading