Correct styles for emoji dropdown #2670

msamsel · 2018-12-11T16:39:26Z

What is the purpose of this pull request?

bug fix

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?

improve style for emoji dropdown:
- group svg icons, are now centred
- Statusbar icon is moved a little bit to bottom, which visually looks much better

Close #2572

engineering-this

Test fails on Edge and IE:

engineering-this · 2018-12-28T13:01:29Z

tests/plugins/emoji/manual/dropdown/outlinestyleandstatusbar.md

+## Test case 1:
+
+1. Open emoji dropdown.
+2. Move focus (blue rectangular border) to `nature and animals` group.


I'd add information on how to focus, eg:
Desktop: Press Shift+Tab, then use arrow keys to navigate to group.
Android: Long touch on group.
iOS: ???

I've ignored this test on mobiles.

engineering-this

Looks good, just two small things.

engineering-this · 2019-01-10T16:00:38Z

CHANGES.md

+
+Fixed Issues:
+
+* [#2572](https://github.com/ckeditor/ckeditor-dev/issues/2572): Fixed: [Emoji](https://ckeditor.com/cke4/addon/emoji) dropdown styles improvement: icons in navigation groups are centred and preview in statusbar has better top margin.


Fixed: Emoji dropdown styles improvement: icons in navigation groups are centred and preview in statusbar has better top margin.

This sounds odd. Improvement is not something that is being fixed.

IMO it should be either: Fixed: dropdown styles: ...
or moved to Other changes section, and changed to: Improved: dropdown styles: ....

engineering-this · 2019-01-10T16:00:46Z

tests/plugins/emoji/dropdown.js

+						assert.areSame( translations[ li.data( 'cke-emoji-group' ) ].y || 0, parseFloat( values[ 2 ] || 0 ),
+							'Translation in Y axis for group: "' + li.data( 'cke-emoji-group' ) + '" is incorrect.' );
+					}
+				} finally {


Do we need try/finally blocks? Panel could be hidden before for loop, and it still should work, as element is still living in DOM tree, it is hidden with CSS style.

try/finally is used in those test to clean up after test case run
Panels are not available in any variable, that's why cleanup in teardown method is not possible. I consider as a good practice to clean up after a test case.
In case that test breaks, error will stop running given test case, so panel windows might remain open. This potentially could influence on further test cases. That's why I use try/finally to have sure that panel window will be closed even though test case fail.

What I meant is to move code from finally block panel.hide(); to be executed right before for loop. This shouldn't break assertions, as elements are still in DOM. And as panel is hidden before asserting then fails wouldn't interfere with any following tests.

See:
https://github.com/ckeditor/ckeditor-dev/blob/b4f20e13fee07ca069db112435c4a9f7868cd79a/tests/plugins/emoji/dropdown.js#L355-L370

Additionally there are three test suites that doesn't cleanup when fail.
https://github.com/ckeditor/ckeditor-dev/blob/t/2572/tests/plugins/emoji/dropdown.js#L74
https://github.com/ckeditor/ckeditor-dev/blob/t/2572/tests/plugins/emoji/dropdown.js#L142
https://github.com/ckeditor/ckeditor-dev/blob/t/2572/tests/plugins/emoji/dropdown.js#L164

Could we correct them?

engineering-this · 2019-01-17T15:29:18Z

tests/plugins/emoji/dropdown.js

+						assert.areSame( translations[ li.data( 'cke-emoji-group' ) ].y || 0, parseFloat( values[ 2 ] || 0 ),
+							'Translation in Y axis for group: "' + li.data( 'cke-emoji-group' ) + '" is incorrect.' );
+					}
+				} finally {


What I meant is to move code from finally block panel.hide(); to be executed right before for loop. This shouldn't break assertions, as elements are still in DOM. And as panel is hidden before asserting then fails wouldn't interfere with any following tests.

See:
https://github.com/ckeditor/ckeditor-dev/blob/b4f20e13fee07ca069db112435c4a9f7868cd79a/tests/plugins/emoji/dropdown.js#L355-L370

Additionally there are three test suites that doesn't cleanup when fail.
https://github.com/ckeditor/ckeditor-dev/blob/t/2572/tests/plugins/emoji/dropdown.js#L74
https://github.com/ckeditor/ckeditor-dev/blob/t/2572/tests/plugins/emoji/dropdown.js#L142
https://github.com/ckeditor/ckeditor-dev/blob/t/2572/tests/plugins/emoji/dropdown.js#L164

Could we correct them?

engineering-this

LGTM 👍

mlewand · 2019-01-23T12:29:49Z

I created a preview link for this branch: https://ckeditor4-preview.ckeditor.com/t/2572/plugins/emoji/samples/emoji.html

As a reference of the "old" implementation, here's the major branch: https://ckeditor4-preview.ckeditor.com/major/plugins/emoji/samples/emoji.html

mlewand

I have a strong impression that several top bar icons remain still misaligned:

Buttons I see issue with are:

just minimal misalignment: flags, travels and places
major misalignment: credit card, symbols

I will also note that bottom part alignment got worse on Win 10:

Compared to the former:

Although it was improved for OSX. Can we have it looking well on both systems?

mlewand · 2019-01-23T13:00:51Z

I'll ask @dkonopka for help here, and verifying my observations as I might be missing something here.

dkonopka · 2019-02-12T14:05:03Z

It's a quite long story (I've been there about a week in ck5), but it's almost impossible to centre svg (especially svg sprites), because every browser, every os, every DPI is an impact in rendering process.

... and we can add another factor which is absolutely positioned dropdown, so if its position is top/bottom/left/right 302.459px each browser is rounding it differently, that's why we have ~1px of difference in svg position 😄

mlewand · 2019-02-12T14:08:44Z

All right, thanks @dkonopka for the clarifications. In that case I'll get back to review.

However the tests should say:

Icon has equalish margin between left and right border.

Rather than:

Icon has equal margin between left and right border.

I'll adjust that during the review.

Based on discussion here: #2670 (comment).

mlewand

FF@win10, Edge@win10 fails:

no svg icons are not displayed in emoji dropdown

This doesn't happen with the latest release: https://ckeditor.com/docs/ckeditor4/latest/examples/mentions.html

…png file to be coherent with that change.

… exists.

… not clean up in case of fialure.

Translation were applied to paths usd in SVG so there are no longer reuired to be checked by test.

msamsel · 2019-02-13T10:48:47Z

There was simple issue in SVG, which breaks its render under Edge, and FF. Chrome somehow ignore this issue.

mlewand

LGTM

I had to adjust the changelog.

msamsel force-pushed the t/2572 branch 2 times, most recently from d3a26a4 to af9803d Compare December 12, 2018 08:48

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

mlewand requested a review from engineering-this December 28, 2018 10:43

engineering-this self-assigned this Dec 28, 2018

engineering-this suggested changes Dec 28, 2018

View reviewed changes

CKEditorBot unassigned engineering-this Dec 28, 2018

msamsel force-pushed the t/2572 branch from af9803d to 8ce76c6 Compare January 9, 2019 10:47

msamsel assigned msamsel and unassigned msamsel Jan 9, 2019

msamsel requested a review from engineering-this January 9, 2019 10:55

engineering-this self-assigned this Jan 10, 2019

engineering-this suggested changes Jan 10, 2019

View reviewed changes

CKEditorBot unassigned engineering-this Jan 10, 2019

msamsel force-pushed the t/2572 branch from 8ce76c6 to 12dd313 Compare January 15, 2019 09:53

msamsel requested a review from engineering-this January 15, 2019 10:06

engineering-this self-assigned this Jan 17, 2019

engineering-this suggested changes Jan 17, 2019

View reviewed changes

engineering-this removed their assignment Jan 21, 2019

msamsel assigned msamsel and unassigned msamsel Jan 21, 2019

msamsel requested a review from engineering-this January 21, 2019 13:31

engineering-this self-assigned this Jan 22, 2019

engineering-this approved these changes Jan 22, 2019

View reviewed changes

CKEditorBot unassigned engineering-this Jan 22, 2019

mlewand self-requested a review January 23, 2019 12:54

mlewand suggested changes Jan 23, 2019

View reviewed changes

msamsel self-assigned this Jan 28, 2019

mlewand self-requested a review February 12, 2019 14:31

mlewand added a commit that referenced this pull request Feb 12, 2019

Clarified that svg used from sprites can not be reliably centered.

1e7ed12

Based on discussion here: #2670 (comment).

mlewand suggested changes Feb 12, 2019

View reviewed changes

msamsel self-assigned this Feb 13, 2019

Mateusz Samsel and others added 15 commits February 13, 2019 11:16

Add transformation to move icon to the center of svg element. Update …

f78105d

…png file to be coherent with that change.

Improve emoji outline styling.

b16ff1f

Improve styling of emoji in status bar.

0375441

Add test case for new styles of emoji dropdown.

45416c8

Correct translation values to be exact same as in svg sprite.

e5b1346

Add unit test, which checks if proper transformation for svg elements…

8d2aba7

… exists.

Fix condition in unit test to work on Edge.

5906ef3

Split manual test into to 2 more meaningful test cases.

4c0f6bc

Add changelog entry.

43d381f

Add chengelog entry.

b0d6baf

Remove try finally block from irrelevant places, fix test which might…

46087af

… not clean up in case of fialure.

Introduced new svg with <symbol> syntax.

3b2ee6f

Remove unit test, which is no longer required.

a640b16

Translation were applied to paths usd in SVG so there are no longer reuired to be checked by test.

Remove unnecessary quote character.

df5ec44

Correct svg syntax.

b87abbb

msamsel force-pushed the t/2572 branch from 1e7ed12 to b87abbb Compare February 13, 2019 10:35

msamsel removed their assignment Feb 13, 2019

msamsel requested a review from mlewand February 13, 2019 10:49

mlewand approved these changes Feb 13, 2019

View reviewed changes

Corrected changelog.

7c6128e

mlewand merged commit 296110e into master Feb 13, 2019

CKEditorBot deleted the t/2572 branch February 13, 2019 14:57

mlewand mentioned this pull request Feb 13, 2019

Emoji dropdown icons are not centered #2572

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct styles for emoji dropdown #2670

Correct styles for emoji dropdown #2670

msamsel commented Dec 11, 2018 •

edited

Loading

engineering-this left a comment

engineering-this Dec 28, 2018

msamsel Jan 9, 2019

engineering-this left a comment

engineering-this Jan 10, 2019

engineering-this Jan 10, 2019 •

edited

Loading

msamsel Jan 15, 2019

engineering-this Jan 17, 2019

engineering-this Jan 17, 2019

engineering-this left a comment

mlewand commented Jan 23, 2019 •

edited

Loading

mlewand left a comment

mlewand commented Jan 23, 2019

dkonopka commented Feb 12, 2019 •

edited

Loading

mlewand commented Feb 12, 2019

mlewand left a comment

msamsel commented Feb 13, 2019

mlewand left a comment •

edited

Loading


		Fixed Issues:

		* [#2572](https://github.com/ckeditor/ckeditor-dev/issues/2572): Fixed: [Emoji](https://ckeditor.com/cke4/addon/emoji) dropdown styles improvement: icons in navigation groups are centred and preview in statusbar has better top margin.

Correct styles for emoji dropdown #2670

Correct styles for emoji dropdown #2670

Conversation

msamsel commented Dec 11, 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?

engineering-this left a comment

Choose a reason for hiding this comment

engineering-this Dec 28, 2018

Choose a reason for hiding this comment

msamsel Jan 9, 2019

Choose a reason for hiding this comment

engineering-this left a comment

Choose a reason for hiding this comment

engineering-this Jan 10, 2019

Choose a reason for hiding this comment

engineering-this Jan 10, 2019 • edited Loading

Choose a reason for hiding this comment

msamsel Jan 15, 2019

Choose a reason for hiding this comment

engineering-this Jan 17, 2019

Choose a reason for hiding this comment

engineering-this Jan 17, 2019

Choose a reason for hiding this comment

engineering-this left a comment

Choose a reason for hiding this comment

mlewand commented Jan 23, 2019 • edited Loading

mlewand left a comment

Choose a reason for hiding this comment

mlewand commented Jan 23, 2019

dkonopka commented Feb 12, 2019 • edited Loading

mlewand commented Feb 12, 2019

mlewand left a comment

Choose a reason for hiding this comment

msamsel commented Feb 13, 2019

mlewand left a comment • edited Loading

Choose a reason for hiding this comment

msamsel commented Dec 11, 2018 •

edited

Loading

engineering-this Jan 10, 2019 •

edited

Loading

mlewand commented Jan 23, 2019 •

edited

Loading

dkonopka commented Feb 12, 2019 •

edited

Loading

mlewand left a comment •

edited

Loading