Auto adding `#` to color from colordialog #580

msamsel · 2017-06-29T11:35:16Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

yes

This PR contains

Unit tests
Manual tests

What changes did you make?

There will be automatically added # to colour in colordialog plugin, if colour will be 3 or 6 hexadecimal digit.

Closes #607.

Comandeer · 2017-06-30T10:25:48Z

plugins/colordialog/dialogs/colordialog.js

@@ -22,6 +22,14 @@ CKEDITOR.dialog.add( 'colordialog', function( editor ) {
 		html: '&nbsp;'


What is the purpose of the changes in this file?

Change allows on writing unit test for this bug-fix.

Generally writing test for it was very tricky. Problem appeared when ok button is pressed then method clearHighlight() is called. What throw an error because unit test doesn't highlight colour in dialog as user does.
So I just add if-statement which check if focus is defined and also rearrange variable declaration to pass JShint.

Color dialog hasn't got any unit test earlier.
This change:

if ( focused ) { focused.removeClass( focusedColorLightCls ); focused.removeClass( focusedColorDarkCls ); }

Allows on making nice unit test which set up values but doesn't touch focused colour. So test don't throw up errors when only value exists without focusing it.

Comandeer · 2017-06-30T10:26:13Z

plugins/colorbutton/plugin.js

@@ -203,6 +203,11 @@ CKEDITOR.plugins.add( 'colorbutton', {
 				total = colors.length + ( moreColorsEnabled ? 2 : 1 );

 			var clickFn = CKEDITOR.tools.addFunction( function applyColorStyle( color, type ) {
+				// #565


Please add short comment explaining the change.

Comandeer · 2017-06-30T10:43:16Z

plugins/colorbutton/plugin.js

@@ -203,6 +203,11 @@ CKEDITOR.plugins.add( 'colorbutton', {
 				total = colors.length + ( moreColorsEnabled ? 2 : 1 );

 			var clickFn = CKEDITOR.tools.addFunction( function applyColorStyle( color, type ) {
+				// #565
+				if ( /^[0-9a-f]{3}([0-9a-f]{3})?$/i.test( color ) ) {
+					color = '#' + color;


IMO the fix is placed in incorrect plugin. I'd move it into colordialog one, inside onClose handler of the dialog.

Comandeer · 2017-06-30T10:44:29Z

tests/plugins/colordialog/collorbuttondialogs.js

@@ -0,0 +1,240 @@
+/* bender-tags: editor,unit */


There's no need for unit tag.

Comandeer · 2017-06-30T10:45:11Z

tests/plugins/colordialog/manual/autohashadding.md

@@ -0,0 +1,14 @@
+@bender-tags: manual, tc, 4.8.0, feature, 565


There's no need for manual tag.

mlewand · 2017-06-30T13:05:47Z

Ok to sum it up:

The fix should land in the colordialog plugin rather than in color button.

There's no question to that, as first of all the enhancement should propagate to all fetarues using colordialog, see "Cell Properties" dialog:

Generally speaking it's not colorbutton's job to mess with color format (there are exception but I'll leave it for sake of simplicity).

This ticket will depend on #590, as AFAICS otherwise we'd have to duplicate this logic in colorbutton plugin.

…ssayry any longer.

f1ames

The solution works fine. Just a few things to polish in tests.

f1ames · 2017-07-03T11:05:14Z

tests/plugins/colordialog/colordialog.js

@@ -0,0 +1,121 @@
+/* bender-tags: editor */


Remember to always put unit tag if it is an unit test.

So we discussed this once again, if unit tag is not needed (not used by bender) it should be skipped.

f1ames · 2017-07-03T11:14:20Z

tests/plugins/colordialog/manual/addhash.md

@@ -0,0 +1,13 @@
+@bender-tags: tc, feature, colordialog, 565, 4.8.0
+@bender-ui: collapsed
+@bender-ckeditor-plugins: table,tabletools,colordialog,toolbar,wysiwygarea,sourcearea,contextmenu


Is sourcearea plugin really needed here?

It might be helpful when you wish to check what was included to attribute of tag. E.g. if there will be some invalid string or colour hard to distinguish from others, then checking source might be helpful.

f1ames · 2017-07-03T11:15:14Z

tests/plugins/colordialog/manual/addhash.md

+1. Choose background color.
+1. Type color in hexadecimal 6-digit format **without** leading `#`. E.g. (`ABC123`).
+1. Confirm it.
+1. Inseted color should contain additional `#` at the beginning.


Typo: Inseted -> Inserted

f1ames · 2017-07-03T11:17:25Z

tests/plugins/colordialog/manual/addhash.md

+1. Inseted color should contain additional `#` at the beginning.
+1. Repeat above steps for color with hexadecimal 3-digits format. E.g. (`FFF`).
+
+Note: You can also check some wrong situations.


It may be not clear what is wrong situation. You could rephrase it like:

You can also check with colors in different formats (rgb, text) or invalid hex strings. For such colors # should not be added.

f1ames · 2017-07-03T11:19:06Z

tests/plugins/colordialog/manual/addhash.md

@@ -0,0 +1,13 @@
+@bender-tags: tc, feature, colordialog, 565, 4.8.0


I think we do not use feature tag (unless it was added recently)?

As it was discussed some time ago there should either bug or feature tag? Or does it change since then?

Maybe I'm not up to date - if it was discussed with @mlewand or @Comandeer in the last 2-3 weeks it means it should be that way.

It was discussed around 12 - 13 June on ckeditor4-chat. I made some kind of sum up for us as pinned post to ckeditor4-chat.

f1ames · 2017-07-03T11:21:21Z

tests/plugins/colordialog/colordialog.js

+
+	bender.editor = {};
+
+	var test = {


There is no need to assign it to a variable, you can just write it asbender.test( { ... } ).

f1ames · 2017-07-03T11:26:36Z

tests/plugins/colordialog/colordialog.js

+		},
+
+		'test colordialog does not add hash empty value': function() {
+			var editor = this.editor;


All tests have very similar body, maybe it could be extracted to a separate function like assertColor( editor, initialColor, expectedColor ) and then each test will be a one liner, e.g. assertColor( this.editor, '123456', '#123456').

If you put the function inside test object you can even omit editor param because of the same scope so this.editor will be available there.

f1ames · 2017-07-03T11:33:40Z

tests/plugins/colordialog/colordialog.js

+			wait();
+		},
+
+		'test colordialog does not add hash 1 digit': function() {


This test fails on IE8.

f1ames

Just three little details to adjust in the tests and it will be ready :)

f1ames · 2017-07-05T07:22:06Z

tests/plugins/colordialog/colordialog.js

+			wait();
+		},
+
+		'test colordialog add hash to colors 6 digits': function() {


I think test names would be more readable like:

test colordialog add hash to colors - 6 digits

test colordialog add hash to colors: 6 digits

test colordialog add hash to colors with 6 digits color

f1ames · 2017-07-05T07:25:39Z

tests/plugins/colordialog/colordialog.js

+		},
+
+		'test colordialog does not add hash 1 digit': function() {
+			// IE8 don't allow on totally wrong values of attributes


IE8 doesn't allowing invalid values means the attribute will not be set? I think it might be more clear with something like:
IE8 doesn't allow to use invalid color values. or IE8 doesn't set invalid color values. ?

f1ames · 2017-07-05T07:39:22Z

tests/plugins/colordialog/manual/addhash.md

+
+1. Right click into table cell.
+1. Select `Cell -> Cell Properties`.
+1. Choose background color.


Does it mean to click Choose button and then type color in a colordialog? It might be a little unclear, and typing color without hash in Cell Properties dialog does not add hash (and color when closing dialog) so it may seem it is broken in this case.
To make it more clear you may use: Click Choose next to Background Color or something similar.

f1ames

LGTM!

Tomasz is off, while all the points from the list has been addressed

msamsel changed the title ~~T/565~~ Auto adding # to color from colordialog Jun 29, 2017

Comandeer self-requested a review June 30, 2017 09:51

Comandeer previously requested changes Jun 30, 2017

View reviewed changes

Mateusz Samsel added 6 commits June 30, 2017 16:10

Bug fix.

508eecb

Adding unit and manual tests.

bc2ed66

Move fix to colordialog from colorbutton.

43d5a42

Revert focus change. Becasue tests will change this one won't be nece…

e339434

…ssayry any longer.

Different solution requires different test.

82cbcda

Adding unit and manual tests.

df8a63b

msamsel force-pushed the t/565 branch from c69e723 to df8a63b Compare June 30, 2017 15:07

f1ames self-requested a review July 3, 2017 09:49

f1ames suggested changes Jul 3, 2017

View reviewed changes

Mateusz Samsel added 3 commits July 4, 2017 13:15

Get back this change. Prevent of throwing errors in test on IE8

dbb8016

Remake test to be oneliners.

e463657

Correct typo and make better note description.

818bd17

f1ames self-requested a review July 5, 2017 07:17

f1ames suggested changes Jul 5, 2017

View reviewed changes

msamsel added this to the 4.8.0 milestone Jul 5, 2017

Update test case names and impove manual test description.

ba184c6

f1ames self-requested a review July 5, 2017 10:13

f1ames approved these changes Jul 5, 2017

View reviewed changes

f1ames force-pushed the t/565 branch from c995bbc to afc13a7 Compare July 5, 2017 10:44

f1ames force-pushed the t/565 branch 2 times, most recently from 4de4d20 to 2b6bd45 Compare July 5, 2017 11:48

f1ames added 2 commits July 5, 2017 13:48

Tests: rephrased manual test description.

dbf8d8e

Changelog entry. [skip ci]

2b6bd45

f1ames merged commit 6ae9c49 into major Jul 5, 2017

f1ames deleted the t/565 branch July 5, 2017 11:51

This was referenced Jul 5, 2017

Manually adding hex color in color picker dialog doesn't pre-fill with hashtag when changing table cell border or background #607

Closed

Manually adding hex color in color picker dialog doesn't pre-fill with hashtag #565

Closed

mlewand removed this from the 4.8.0 milestone Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto adding `#` to color from colordialog #580

Auto adding `#` to color from colordialog #580

msamsel commented Jun 29, 2017 •

edited by f1ames

Loading

Comandeer Jun 30, 2017

msamsel Jun 30, 2017

msamsel Jul 5, 2017

Comandeer Jun 30, 2017

Comandeer Jun 30, 2017

Comandeer Jun 30, 2017

Comandeer Jun 30, 2017

mlewand commented Jun 30, 2017

f1ames left a comment

f1ames Jul 3, 2017

f1ames Jul 4, 2017

f1ames Jul 3, 2017

msamsel Jul 4, 2017

f1ames Jul 3, 2017

f1ames Jul 3, 2017

f1ames Jul 3, 2017

msamsel Jul 4, 2017

f1ames Jul 4, 2017

msamsel Jul 4, 2017

f1ames Jul 3, 2017

f1ames Jul 3, 2017

f1ames Jul 3, 2017

f1ames left a comment

f1ames Jul 5, 2017

f1ames Jul 5, 2017

f1ames Jul 5, 2017

f1ames left a comment

		@@ -22,6 +22,14 @@ CKEDITOR.dialog.add( 'colordialog', function( editor ) {
		html: ' '

		@@ -0,0 +1,14 @@
		@bender-tags: manual, tc, 4.8.0, feature, 565

		@@ -0,0 +1,13 @@
		@bender-tags: tc, feature, colordialog, 565, 4.8.0

Auto adding # to color from colordialog #580

Auto adding # to color from colordialog #580

Conversation

msamsel commented Jun 29, 2017 • edited by f1ames Loading

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

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 Jun 30, 2017

f1ames 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames 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

f1ames left a comment

Choose a reason for hiding this comment

Auto adding `#` to color from colordialog #580

Auto adding `#` to color from colordialog #580

msamsel commented Jun 29, 2017 •

edited by f1ames

Loading