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.

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

Replace emojione with twemoji + emojibase #2995

Merged
merged 17 commits into from
May 21, 2019
Merged

Replace emojione with twemoji + emojibase #2995

merged 17 commits into from
May 21, 2019

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented May 19, 2019

@ara4n
Copy link
Member Author

ara4n commented May 19, 2019

Right, this should generally work. Known problems are:

Screenshot 2019-05-19 at 21 03 27

where the black square is a default layer glyph after my antics on https://github.com/mozilla/twemoji-colr/issues/50

Screenshot 2019-05-19 at 22 23 45

  • the emojibase shortcodes (e.g. :smile: etc) are different to emojione's, so they've all changed, sometimes rather unexpectedly. For instance, :smile: produces a grin of some kind, whereas :pleased: produces a more normal smiley... (@jryans: I think we've managed to just deal with the change.)
  • we no longer get tooltips on emoji giving their shortcodes, as they are no longer images. This would need to be implemented with some additional JS (to avoid adding additional spans for emoji everywhere, which would take us back to where we were with emojione) (Filed as Emoji shortcode is missing on hover element-hq/element-web#9911.)
  • any emoji glyphs which Nunito randomly decides to implement will take priority over Twemoji's glyphs. I haven't found any though. This may be more of a problem for whatever the monospace font is, especially as this is platform dependent currently, as per we don't define a consistent cross-platform monospace font element-hq/element-web#9765
  • i deliberately reverted 9a272d4 given it applies the full emoji regexp to every keystroke in the composer, slowing things down. This means that users who have autocomplete turned on can't do sequences of emojis, but i think that regression is fine given the perf improvement.
  • performancewise the autocompleter shouldn't have regressed, but it's nuts that we return 1724 autocomplete results every time someone types a colon :/
  • twemoji-colr is at Emoji 11 (whereas 12 is latest from twemoji). We should get this for free once Update to Twemoji v12 mozilla/twemoji-colr#48 lands.
  • Zooming the browser with command+ results in ugly raster sampling artefacts on Chrome. Firefox however correctly renders as vectors. (@jryans: I wasn't able to duplicate this in Chrome, will wait for others to report.)
  • the current twitter emoji artwork has a slightly more cartoony and perhaps clunky aesthetic relative to emojione. this is only really a problem for bigemoji though.

I don't think any of these are hard blockers on merging this.

@ara4n ara4n marked this pull request as ready for review May 19, 2019 20:11
@ara4n ara4n requested a review from a team May 19, 2019 20:11
@turt2live
Copy link
Member

out of interest, does this magically solve element-hq/element-web#7872 (I suspect it doesn't, but would be cool if it did)

@ara4n
Copy link
Member Author

ara4n commented May 19, 2019

i doubt it will. in fact, it might make the fix harder, as text in general will need to be de-inverted whereas before the emoji CSS could be de-inverted. the correct fix there really is to stop using the invert filter hack.

@jryans jryans requested a review from a team May 20, 2019 09:04
src/HtmlUtils.js Outdated Show resolved Hide resolved
@jryans jryans requested a review from a team May 20, 2019 09:09
@jryans
Copy link
Collaborator

jryans commented May 20, 2019

I've only done a quick scan out of curiosity... 😁 @bwindels mentioned he'd do a proper review during our morning standup.

@bwindels bwindels self-requested a review May 20, 2019 15:23
@bwindels
Copy link
Contributor

Started reviewing, will continue tomorrow ...

Copy link
Contributor

@bwindels bwindels left a comment

Choose a reason for hiding this comment

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

Apart from what looks like including two json files with emoji data in the bundle, looks great! Tried it out as well and OMG this feels so much nicer!!

src/HtmlUtils.js Show resolved Hide resolved
src/HtmlUtils.js Show resolved Hide resolved
@ara4n
Copy link
Member Author

ara4n commented May 21, 2019

@bwindels thanks for the review :) the two points are basically things i'd deliberately skipped for now to avoid potentially lost work given the questions over how emojipicking will work in future. do you consider them hard blockers?

@bwindels
Copy link
Contributor

bwindels commented May 21, 2019

No, no blockers. If we don't end up switching to emojimart for some reason we'll need to remember to come back to this, but I don't see why we wouldn't, so all good to go 👍

@ara4n
Copy link
Member Author

ara4n commented May 21, 2019

If we don't end up switching to emojimart for some reason we'll need to remember to come back to this

have filed element-hq/element-web#9780 to track the stripped emoji mess.

@jryans
Copy link
Collaborator

jryans commented Jun 17, 2019

I extracted the bits of #2995 (comment) that seemed like that really need follow up to separate issues which I've added into the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove emojione
4 participants