Add the strikethrough feature. #58

Natim · 2017-11-17T15:49:22Z

Following discussions in mozilla/notes#397 I am adding the prelimilary work around the strikethrough feature. I would need a SVG icon for the default theme.

Natim · 2017-11-17T15:51:49Z

By the way feel free to send me the CLA I would be glad to sign it.

Natim · 2017-11-17T15:52:42Z

src/strike.js

+ */
+
+/**
+ * @module notes/ckeditor-build/strike


This should change.

Natim · 2017-11-17T15:52:52Z

src/strikeengine.js

+ */
+
+/**
+ * @module notes/ckeditor-build/strikeengine


This should change.

Reinmar · 2017-11-17T15:52:59Z

Thanks!

To merge it, we'll also need an:

automated tests (see tests/),
adding underline to the manual test (see tests/manual/),

You can read about our test environment or, since this is a simple feature, just copy&paste&hopeThatItWorks :).

Natim · 2017-11-17T15:53:43Z

src/strike.js

+		const editor = this.editor;
+		const t = editor.t;
+		const command = editor.commands.get( 'strike' );
+		const keystroke = 'CTRL+E';


Please advice of a good keystroke for that feature. I tried CTRL+ALT+S but it doesn't seems to work.

cc @Comandeer

I don't know why ctrl+alt+s didn't work so maybe there's some problem with https://github.com/ckeditor/ckeditor5-utils/blob/master/src/keyboard.js#L81 ;|

Editor Recommendations does not define shortcut for strikethorugh, however it's probably good to stick to what Word does as it's a de facto standard: Cmd + Shift + X in Office 365 on macOS.

@Reinmar @Comandeer what font do y'all use for icons?

Can I use Cmd+Shift+X in the keystroke var here? Will the editor understand Cmd or should I use Meta?

Right now typing Cmd key remove the selected content.

@Natim Should be Meta I think

Not according to https://github.com/ckeditor/ckeditor5-utils/blob/master/src/keyboard.js#L25

Reinmar · 2017-11-17T15:53:58Z

By the way feel free to send me the CLA I would be glad to sign it;

It should be sent automatically. Let us know if you don't get it.

Natim · 2017-11-17T18:21:09Z

Folks I am going to be off for the next 12 hours, feel free to take over the branch if you want to update the icon or the key stroke. Tests were passing with CTRL+E.

I would be glad to take your reviews as well.

oleq · 2017-11-20T10:03:13Z

I pushed an improved strike icon. Please note that it will be refactored in ckeditor/ckeditor5#645 anyway.

Natim · 2017-11-20T10:10:10Z

Nice thanks !! Any idea of how to fix the shortcut? Should I remove it for now?

Reinmar · 2017-11-20T11:14:54Z

I've been a bit unsure about the view element name – whether it should be s or del but I can see that in Editor Recommendations we agreed that s makes most sense and it's also the most commonly used: ckeditor/editor-recommendations#3.

Reinmar · 2017-11-20T11:17:43Z

Nice thanks !! Any idea of how to fix the shortcut? Should I remove it for now?

We need to make cmd+shift+x work. So if you previously had a problem with cmd+alt+a, then there are two options now:

switching to cmd+shift+x will work just fine out of the box because the problem was caused by the browser not letting us handle cmd+alt+a,
there's some problem with utils/keyboard.js which is not able to handle a keystroke with two modifiers for some reason (in which case we need to look into that).

szymonkups · 2017-11-20T11:57:18Z

src/strike.js

+import strikeIcon from '../theme/icons/strike.svg';
+
+/**
+ * The strike feature. It introduces the Strike button and the <kbd>Ctrl+E</kbd> keystroke.


Please change the keystroke to currently used one.

szymonkups · 2017-11-20T12:04:29Z

src/strike.js

+		const editor = this.editor;
+		const t = editor.t;
+		const command = editor.commands.get( 'strike' );
+		const keystroke = 'Cmd+Shift+X';


It is working for me without any problems (maybe OS is blocking it on your machine?). Please use CTRL+SHIFT+X (uppercase and CTRL instead of CMD). It will be consistent with the way we define it in other features. Note that this will also work with command key as we internally handle that.

I don't think CMD and CTRL are the same key, one is Ctrl the other one is Meta isn't it?

They are not same key, we just treat them that way, and you just need to define keystroke for CTRL and it will be handled correctly on Mac. Please see: http://ckeditor.github.io/editor-recommendations/general-policies/#ctrl-vs-cmd.

szymonkups · 2017-11-20T12:07:10Z

src/strike.js

+			const view = new ButtonView( locale );
+
+			view.set( {
+				label: t( 'Strike' ),


Please add Strike to the lang/contexts.json. This will give a context to the translators.

szymonkups · 2017-11-20T12:10:31Z

tests/manual/basic-styles.md

  * underline "instance",
-  * code "is an".


You should also update tests/manual/basic-styles.html to reflect these changes.

szymonkups · 2017-11-20T12:31:47Z

tests/strike.js

+
+	it( 'should set editor keystroke', () => {
+		const spy = sinon.spy( editor, 'execute' );
+		const keyEventData = {


This test fails. It uses alt key instead of shift key. After unifying keystroke to CTRL+SHIFT+X, keyEventData should look like this:

const keyEventData = { keyCode: keyCodes.x, shiftKey: true, ctrlKey: true, preventDefault: sinon.spy(), stopPropagation: sinon.spy() };

It isn't Ctrl but Cmd as far as I am aware

szymonkups · 2017-11-20T12:37:21Z

Do we want the feature to be named "Strike" and not "Strikethrough"?

Natim · 2017-11-20T13:01:44Z

Do we want the feature to be named "Strike" and not "Strikethrough"?

I initially named it Strikethrough but renamed it when I saw that in CKeditor 4 it was called Strike.

wwalc · 2017-11-20T14:07:11Z

I'd go with Strikethrough as the feature itself will render <s>, not the <strike> element (as one would suspect). This is also the name of the feature that we use in the Editor Recommendations project, so sticking to strike seems to be kinda a lack of consequence.

Natim · 2017-11-20T14:10:07Z

Ok I will make the change then 👍

szymonkups · 2017-11-20T14:30:32Z

src/strikethrough.js

@@ -0,0 +1,68 @@
+/**
+ * @license Copyright (c) 2017, CKSource - Rémy Hubscher. All rights reserved.


We are not modifying standard header for each developer. "CKSource - Frederico Knabben" it's our company full name. We really appreciate your contribution, it will be noted in GitHub history and our change log, but we cannot change copyright information - this is also why we needed CLA to be signed.

@Natim - are you ok with me modifying headers back to original state and merging this PR?

Sorry I forgot about that, it should be fixed now.

vladikoff · 2017-11-20T15:12:39Z

theme/icons/strikethrough.svg

@@ -0,0 +1 @@
+<svg viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg" fill-rule="evenodd" clip-rule="evenodd" stroke-linejoin="round" stroke-miterlimit="1.414"><path d="M8.396 10.861c1.83.02 3.66.038 5.489.014.311.54.466 1.147.466 1.823 0 .944-.373 1.834-1.119 2.671-.745.837-1.94 1.255-3.584 1.255a6.429 6.429 0 0 1-2.291-.429l-.366-.137a2.901 2.901 0 0 0-.246-.059 1.113 1.113 0 0 0-.201-.023c-.195 0-.329.056-.402.169a1.704 1.704 0 0 0-.192.479h-.538v-4.629h.538c.305 1.394.796 2.408 1.475 3.041s1.456.949 2.333.949c.852 0 1.458-.217 1.817-.653.359-.435.539-.893.539-1.374 0-.56-.189-1.016-.566-1.369-.25-.232-.752-.533-1.507-.904l-1.214-.603a15.578 15.578 0 0 1-.431-.221zm2.881-2.034l.535.264c.459.228.85.473 1.172.736H6.806a4.395 4.395 0 0 1-.527-.516 2.98 2.98 0 0 1-.334-.484h5.332zm2.526-1.379h-.521a5.656 5.656 0 0 0-1.319-2.438c-.63-.682-1.395-1.023-2.296-1.023-.633 0-1.134.172-1.502.516a1.653 1.653 0 0 0-.553 1.256c0 .59.174 1.032.521 1.324.194.169.523.385.987.645-1.177.006-2.355.023-3.532.053a4.065 4.065 0 0 1-.048-.634c0-.913.327-1.766.981-2.557.654-.791 1.657-1.187 3.009-1.187a6.46 6.46 0 0 1 1.995.31c.642.207 1.009.311 1.1.311.207 0 .35-.059.429-.178a1.53 1.53 0 0 0 .192-.47h.557v4.072z" fill="#454545"/><path fill="#454545" d="M4 8.827h12v1H4z"/></svg>


@Natim just to confirm this is a different icon now right? not the one you got from the internet? 😄

Yes that's @oleq one.

vladikoff · 2017-11-20T18:30:31Z

CI fail with:

Chrome 62.0.3202 (Linux 0.0.0) ERROR
  Uncaught Error: Cannot find module "../src/strike"
  at tests/strikethrough.js:53179
Chrome 62.0.3202 (Linux 0.0.0): Executed 0 of 0 ERROR (1.17 secs / 0 secs)
Coverage report saved in '/home/travis/build/ckeditor/ckeditor5-basic-styles/coverage'.

vladikoff · 2017-11-21T01:40:47Z

src/strikethrough.js

+		const editor = this.editor;
+		const t = editor.t;
+		const command = editor.commands.get( 'strikethrough' );
+		const keystroke = 'CTRL+SHIFT+X';


"""
Firefox: Set direction using the CTRL/CMD+SHIFT+X keyboard shortcut, which cycles through LTR and RTL. It does not set the value of the element's dir attribute, and is thus invisible to scripts.
"""

From https://www.w3.org/International/questions/qa-html-dir

We somehow got into this issue here: mozilla/notes#409

vladikoff · 2017-11-21T01:46:50Z

src/strikethrough.js

+			view.set( {
+				label: t( 'Strikethrough' ),
+				icon: strikethroughIcon,
+				keystroke,


It our version the tooltip does not show "X" in the tooltip? is this also affected by this?

@vladikoff I check that out and I found out it's a bug https://github.com/ckeditor/ckeditor5-utils/issues/206.

@szymonkups Could you check if ckeditor/ckeditor5-utils#207 fixes the issue?

@oleq - checked, tested and merged :)

vladikoff · 2017-12-05T17:54:20Z

Awesome!!

Add the strikethrough feature.

5de488a

Natim commented Nov 17, 2017

View reviewed changes

src/strike.js Outdated

*/

/**

* @module notes/ckeditor-build/strike

Copy link

Author

Natim Nov 17, 2017

Choose a reason for hiding this comment

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

This should change.

Natim commented Nov 17, 2017

View reviewed changes

src/strikeengine.js Outdated

*/

/**

* @module notes/ckeditor-build/strikeengine

Copy link

Author

Natim Nov 17, 2017

Choose a reason for hiding this comment

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

This should change.

Natim commented Nov 17, 2017

View reviewed changes

Add tests.

a522c17

Natim mentioned this pull request Nov 17, 2017

Add strike plugin. mozilla/notes#397

Merged

Rémy HUBSCHER added 3 commits November 17, 2017 18:54

Add temporary icon.

b5b19ca

Use Cmd+Shift+X keystroke.

8fc5b99

Update Modules names

18fb556

Improved the strikethrough icon.

02cc56e

szymonkups suggested changes Nov 20, 2017

View reviewed changes

Rémy HUBSCHER added 3 commits November 20, 2017 13:58

@szymonkups review.

890c792

Update HTML test.

ae47ebc

Add Strike to the lang context.

cf3ba7e

Rémy HUBSCHER added 2 commits November 20, 2017 15:13

Rename strike to strikethrough.

b770d65

Leftover.

1def162

szymonkups reviewed Nov 20, 2017

View reviewed changes

szymonkups approved these changes Nov 20, 2017

View reviewed changes

vladikoff reviewed Nov 20, 2017

View reviewed changes

vladikoff reviewed Nov 21, 2017

View reviewed changes

Rémy HUBSCHER added 5 commits November 21, 2017 10:15

@vladikoff review.

bfda441

Rename strike attribute.

4312bc7

Rename strike action.

0bd0a2d

Fix tests.

6909875

Update Copyright.

69e6a79

szymonkups merged commit 78719c9 into ckeditor:master Dec 5, 2017

		@@ -0,0 +1,68 @@
		/**
		* @license Copyright (c) 2017, CKSource - Rémy Hubscher. All rights reserved.

		@@ -0,0 +1 @@
		<svg viewBox="0 0 20 20" xmlns="http://www.w3.org/2000/svg" fill-rule="evenodd" clip-rule="evenodd" stroke-linejoin="round" stroke-miterlimit="1.414"><path d="M8.396 10.861c1.83.02 3.66.038 5.489.014.311.54.466 1.147.466 1.823 0 .944-.373 1.834-1.119 2.671-.745.837-1.94 1.255-3.584 1.255a6.429 6.429 0 0 1-2.291-.429l-.366-.137a2.901 2.901 0 0 0-.246-.059 1.113 1.113 0 0 0-.201-.023c-.195 0-.329.056-.402.169a1.704 1.704 0 0 0-.192.479h-.538v-4.629h.538c.305 1.394.796 2.408 1.475 3.041s1.456.949 2.333.949c.852 0 1.458-.217 1.817-.653.359-.435.539-.893.539-1.374 0-.56-.189-1.016-.566-1.369-.25-.232-.752-.533-1.507-.904l-1.214-.603a15.578 15.578 0 0 1-.431-.221zm2.881-2.034l.535.264c.459.228.85.473 1.172.736H6.806a4.395 4.395 0 0 1-.527-.516 2.98 2.98 0 0 1-.334-.484h5.332zm2.526-1.379h-.521a5.656 5.656 0 0 0-1.319-2.438c-.63-.682-1.395-1.023-2.296-1.023-.633 0-1.134.172-1.502.516a1.653 1.653 0 0 0-.553 1.256c0 .59.174 1.032.521 1.324.194.169.523.385.987.645-1.177.006-2.355.023-3.532.053a4.065 4.065 0 0 1-.048-.634c0-.913.327-1.766.981-2.557.654-.791 1.657-1.187 3.009-1.187a6.46 6.46 0 0 1 1.995.31c.642.207 1.009.311 1.1.311.207 0 .35-.059.429-.178a1.53 1.53 0 0 0 .192-.47h.557v4.072z" fill="#454545"/><path fill="#454545" d="M4 8.827h12v1H4z"/></svg>

Add the strikethrough feature. #58

Add the strikethrough feature. #58

Conversation

Natim commented Nov 17, 2017 • edited Loading

Natim commented Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Nov 17, 2017

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

Reinmar commented Nov 17, 2017

Natim commented Nov 17, 2017

oleq commented Nov 20, 2017

Natim commented Nov 20, 2017 • edited Loading

Reinmar commented Nov 20, 2017

Reinmar commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Natim Nov 20, 2017 • 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

szymonkups commented Nov 20, 2017

Natim commented Nov 20, 2017

wwalc commented Nov 20, 2017

Natim commented Nov 20, 2017

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

vladikoff commented Nov 20, 2017

Choose a reason for hiding this comment

vladikoff Nov 21, 2017 • 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

vladikoff commented Dec 5, 2017

Natim commented Nov 17, 2017 •

edited

Loading

Natim commented Nov 17, 2017 •

edited

Loading

Natim commented Nov 20, 2017 •

edited

Loading

Natim Nov 20, 2017 •

edited

Loading

vladikoff Nov 21, 2017 •

edited

Loading