Skip to content

Fix #4149, fix #1199 - Store emojis as unicode#4189

Merged
Gargron merged 1 commit intomasterfrom
fix-gargamel-emojos
Jul 14, 2017
Merged

Fix #4149, fix #1199 - Store emojis as unicode#4189
Gargron merged 1 commit intomasterfrom
fix-gargamel-emojos

Conversation

@Gargron
Copy link
Copy Markdown
Member

@Gargron Gargron commented Jul 13, 2017 •
edited
Loading

  • Use unicode when selecting emoji through picker
  • Convert shortcodes to unicode when storing text input server-side
  • Do not convert shortcodes in JS anymore

Reference: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/10647

@Gargron Gargron force-pushed the fix-gargamel-emojos branch 3 times, most recently from 413e2e5 to d755f49 Compare July 14, 2017 01:38
sorin-davidoi
sorin-davidoi reviewed Jul 14, 2017
View reviewed changes
app/javascript/mastodon/emoji.js Outdated
@@ -11,31 +11,13 @@ function emojify(str) {
// The goal is to be the same as an emojione.regShortNames/regUnicode replacement, but faster.
Copy link
Copy Markdown
Contributor

@sorin-davidoi sorin-davidoi Jul 14, 2017

Choose a reason for hiding this comment

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

The description is outdated.

@sorin-davidoi
Copy link
Copy Markdown
Contributor

sorin-davidoi commented Jul 14, 2017

I'm wondering, shouldn't we make the emoji picker insert unicode instead of shortcodes? I'm looking into tree shaking EmojiOne and if we take out the shortcode substitution in the compose form (which basically pull in the entire list of shortcodes), we can shrink it by ~90% (from 288 KB to 32 KB).

@Gargron
Copy link
Copy Markdown
Member Author

Gargron commented Jul 14, 2017

I'm wondering, shouldn't we make the emoji picker insert unicode instead of shortcodes?

@sorin-davidoi I did that in this PR. So the emoji picker inserts unicode, but if you type out the shortcode, it also gets converted to unicode server-side.

@sorin-davidoi
Copy link
Copy Markdown
Contributor

sorin-davidoi commented Jul 14, 2017

Ah, missed that! Awesome, will follow this with the tree shaking!

@Gargron
Fix #4149, fix #1199 - Store emojis as unicode
555a43e 
- Use unicode when selecting emoji through picker
- Convert shortcodes to unicode when storing text input server-side
- Do not convert shortcodes in JS anymore
@Gargron Gargron force-pushed the fix-gargamel-emojos branch from d755f49 to 555a43e Compare July 14, 2017 13:04
@sorin-davidoi sorin-davidoi mentioned this pull request Jul 14, 2017
Merged
@Gargron Gargron merged commit e2685cc into master Jul 14, 2017
@Gargron Gargron deleted the fix-gargamel-emojos branch July 14, 2017 17:48
@Gargron Gargron mentioned this pull request Jul 15, 2017
Closed
@unarist
Copy link
Copy Markdown
Contributor

unarist commented Jul 16, 2017 •
edited
Loading

After this change, remote contents and also existing local contents which contains :shortname form won't be emoji?

@joyeusenoelle joyeusenoelle mentioned this pull request Jul 24, 2017
Closed
2 tasks
rinsuki added a commit to cinderella-project/iMast that referenced this pull request Jul 3, 2021
@rinsuki
refactor!: remove really old unicode emoji shortcode conversion
a68b33c 
really old mastodon WebUI converts :shortcode: to Unicode emoji (but actually rendered by emojione), but this behaviour is removed in Mastodon 1.5.0rc1 (mastodon/mastodon#4189 that is 4 years ago omg), so we don't need compatible with this behaviour anymore.

also this old-style emojify is only called in some minor points in now and not called in important point (like posts text), so users can't feel this change without notice.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@sorin-davidoi sorin-davidoi sorin-davidoi left review comments

@beatrix-bitrot beatrix-bitrot beatrix-bitrot approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Gargron @sorin-davidoi @unarist @beatrix-bitrot