Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Show matched emoji name instead of an ID in emoji suggestion box #2656

Merged
merged 22 commits into from
Nov 19, 2019
Merged

Show matched emoji name instead of an ID in emoji suggestion box #2656

merged 22 commits into from
Nov 19, 2019

Conversation

engineering-this
Copy link
Contributor

@engineering-this engineering-this commented Dec 6, 2018 •
edited by jacekbogdanski
Loading

What is the purpose of this pull request?

New feature/task

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?

Changed emoji suggestion box item template to show item.name instead of item.id.

Changelog

* [#2583](https://github.com/ckeditor/ckeditor-dev/issues/2583): Changed [Emoji](https://ckeditor.com/cke4/addon/emoji) suggestion box to show matched emoji name instead of an ID.

Closes #2583

@mlewand mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Dec 17, 2018
@mlewand mlewand requested a review from msamsel December 17, 2018 10:00
@mlewand
Copy link
Contributor

mlewand commented Dec 17, 2018

@msamsel please make an early review

@msamsel msamsel self-assigned this Dec 17, 2018
msamsel
msamsel suggested changes Dec 17, 2018
View reviewed changes
Copy link
Contributor

@msamsel msamsel left a comment

Choose a reason for hiding this comment

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

  • what with "smiley icon" which is mentioned by wwalc in task description:

    The current way of presenting smileys looks pretty ugly

    It seems to be font issue there.

tests/plugins/emoji/manual/emojimatchlabel.md Outdated Show resolved Hide resolved
plugins/emoji/plugin.js Outdated Show resolved Hide resolved
plugins/emoji/plugin.js Outdated Show resolved Hide resolved
@msamsel msamsel removed their assignment Dec 17, 2018
@engineering-this engineering-this self-assigned this Dec 18, 2018
msamsel
msamsel suggested changes Dec 19, 2018
View reviewed changes
Copy link
Contributor

@msamsel msamsel left a comment

Choose a reason for hiding this comment

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

There are still some improvements which might be implemented with this feature.

tests/plugins/emoji/manual/emojimatchlabel.html Outdated Show resolved Hide resolved
plugins/emoji/plugin.js Outdated Show resolved Hide resolved
tests/plugins/emoji/basic.js Outdated Show resolved Hide resolved
@engineering-this engineering-this self-assigned this Dec 19, 2018
msamsel
msamsel suggested changes Jan 7, 2019
View reviewed changes
Copy link
Contributor

@msamsel msamsel left a comment

Choose a reason for hiding this comment

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

There remain 2 small code styling issues, besides that everything is ok from my perspective.

tests/plugins/emoji/manual/emojimatchlabel.html Outdated Show resolved Hide resolved
tests/plugins/emoji/manual/font.html Outdated Show resolved Hide resolved
@engineering-this engineering-this self-assigned this Jan 10, 2019
msamsel
msamsel approved these changes Jan 14, 2019
View reviewed changes
Copy link
Contributor

@msamsel msamsel left a comment

Choose a reason for hiding this comment

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

LGTM 👍
From my perspective everything is ok. :)

@mlewand mlewand self-requested a review January 14, 2019 16:05
@mlewand mlewand self-assigned this Jan 14, 2019
@jacekbogdanski jacekbogdanski self-assigned this Nov 13, 2019
jacekbogdanski
jacekbogdanski requested changes Nov 14, 2019
View reviewed changes
Copy link
Member

@jacekbogdanski jacekbogdanski left a comment

Choose a reason for hiding this comment

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

Overall changes look good. I'm only wondering if the current emoji format is fine. After changes, _ is replaced with empty space. It looks nice but when you start typing, you find out that actually, you cannot hit space to continue emoji - you still need to type _ to match emoji ID.

I propose to stick to snake case format, otherwise, it looks like autocomplete just lies about correct match format.

The proposed format in this PR:
image

My proposition:
image

Also, note that my proposition looks the same as e.g. on GH, which also makes it more convenient for the users (also Slack and similar apps uses underscore here).

Please, vote:

  • Underscores (smiling_face) - 👍
  • Spaces (smiling face) - 👎

or share your own proposition.

@jacekbogdanski jacekbogdanski removed the request for review from mlewand November 14, 2019 11:41
@jacekbogdanski
Copy link
Member

Due to lacking PR author, I refactored and updated proposed changes with snake_case format. @msamsel could I ask you to take a second look at this PR, so my changes are also reviewed?

@msamsel msamsel self-assigned this Nov 15, 2019
msamsel
msamsel approved these changes Nov 19, 2019
View reviewed changes
Copy link
Contributor

@msamsel msamsel left a comment

Choose a reason for hiding this comment

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

Ok it looks fine for me. 👍

engineering-this and others added 21 commits November 19, 2019 13:45
@engineering-this @jacekbogdanski
Tests: added test case.
28e338e
@engineering-this @jacekbogdanski
Tests: added manual test.
4ed806d
@engineering-this @jacekbogdanski
Tests: added manual test.
c63bb93
@engineering-this @jacekbogdanski
Emoji: autocomplete panel will show emoji name instead of emoji id.
b41e529
@engineering-this @jacekbogdanski
Tests: corrected tags.
c411f0b
@engineering-this @jacekbogdanski
Tests: added test cases for emoji names cache.
59b8046
@engineering-this @jacekbogdanski
Cached emoji names. Renamed getEmojiName to getEncodedName.
1a10e1c
@engineering-this @jacekbogdanski
Tests: updated test steps, removed redundant editor name.
1bcf633
@engineering-this @jacekbogdanski
Tests: added manual test.
07812c4
@engineering-this @jacekbogdanski
Moved emoji preview to span, added styles to display it correctly.
4598af0
@engineering-this @jacekbogdanski
Tests: removed EmojiPanel button.
4f29168
@engineering-this @jacekbogdanski
Tests: changed element.$.innerText to element.getTexT() in assertion.
3e40cf4
@engineering-this @jacekbogdanski
Reworked getEncodedName to addEncodedName, now function only adds mis…
e15d209 
…sing _name property, but doesn't return anything.
@engineering-this @jacekbogdanski
Tests: corrected codestyle.
9a2e4fe
@jacekbogdanski
Updated tests.
3ef13cb
@jacekbogdanski
Refactoring.
a6fb8e8
@jacekbogdanski
Updated test to use name instead of ID.
42d9604
@jacekbogdanski
Updated emoji name.
93059e9
@jacekbogdanski
Updated test description to contain underscores.
8b80bf7
@jacekbogdanski @msamsel
Rewording.
fe47c28 
Co-Authored-By: Mateusz Samsel <[email protected]>
@jacekbogdanski
Improved tests.
4de9296
jacekbogdanski
jacekbogdanski approved these changes Nov 19, 2019
View reviewed changes
Copy link
Member

@jacekbogdanski jacekbogdanski left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jacekbogdanski jacekbogdanski merged commit 597b149 into major Nov 19, 2019
@CKEditorBot CKEditorBot deleted the t/2583 branch November 19, 2019 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mlewand mlewand mlewand left review comments

@jacekbogdanski jacekbogdanski jacekbogdanski approved these changes

@msamsel msamsel msamsel approved these changes

Assignees
No one assigned
Labels
review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Emoji tooltip should show a human friendly label
4 participants
@engineering-this @mlewand @jacekbogdanski @msamsel