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

[IMPROVE] Upgrade EmojiOne to JoyPixels 4.5.0 #13807

Merged
merged 67 commits into from
May 20, 2019

Conversation

wreiske
Copy link
Contributor

@wreiske wreiske commented Mar 20, 2019

This is READY TO MERGE!

For latest, see #13807 (comment)

Screen Shot 2019-03-20 at 3 40 17 AM

Screen Shot 2019-03-20 at 3 45 03 AM

Closes #13686

Still TODO

Notes

I modified the generateEmojiIndex script to automatically generate the emojiPicker.js file based off the emoji.json from emojione's github tagged 4.5.0.

Need to figure out how @sampaiodiego did the percentage mustache to allow resizing of the emojis without whitespace. zoom and transform: scale(x) do not work on all browsers (transform leaves whitespace)

Firefox:
image

Webkit:
image

I'm thinking about including the 64x sprites and the 32x or 24x... The inline message emojis would use the 24x sprites, while the emoji replies (no other text) would use the 64x sprites (or maybe a size smaller.)

wreiske added 4 commits March 20, 2019 03:49
This is a work in progress commit to update Rocket.Chat to use the latest release of EmojiOne (4.5.0).

Current Issues:
Need to generate percent sprites instead of using zoom and transforms
Need to check and fix various things like emoji reactions, avatars, etc.
@wreiske
Copy link
Contributor Author

wreiske commented Mar 20, 2019

Added attribution text in 4aa6564

image

@engelgabriel
Copy link
Member

@wreiske this is awesome!

@sampaiodiego do you think we can use the assets directly from the npm module or do we have to copy them to public/packages/emojione/assets/sprites/? Ideally, we should avoid having to copy assets and keep them updated manually.

@rodrigok what do you think?

@rodrigok
Copy link
Member

@engelgabriel AFAIK there is no way to provide assets from npm packages, we always need to copy them to the public folder.

@wreiske
Copy link
Contributor Author

wreiske commented Mar 20, 2019

image

I fixed reactions in a local copy, but I'm not going to push it because it is counterproductive. Once we generate the percentages for the sprites, it will fix itself.

As for the assets, we could write a hook that copies the assets from the node modules to the public folder, or I could make an update_emojione.js that goes and pulls the latest sprites from their github. 🤔

Also: I'm noticing aliasing with the current sprite sheet since it's so big (64x) and being zoomed smaller.
image

Are you against having different sized sprite sheets? This would resolve the need for percentage sprites, too. If we added all of their available sprite sheet sizes (24, 32, 40, 64) it would add (57.3 MB) to the release size. These emojis also include sprite@2x.png sizes for retina screens (looks really good on my mbp!)

@engelgabriel
Copy link
Member

I guess we can use the SVG version?

@wreiske
Copy link
Contributor Author

wreiske commented Mar 20, 2019

I guess we can use the SVG version?

I think the free license they are modifying for Rocket.Chat does not include high-quality SVG assets, just the PNG sprites.

It does have font files for ios, mac, and android.
image

@engelgabriel
Copy link
Member

I think the free license they are modifying for us does not include high-quality SVG assets, just the PNG sprites.

I agree, but maybe it is worth double checking? 😬

@wreiske
Copy link
Contributor Author

wreiske commented Mar 20, 2019

I think the free license they are modifying for us does not include high-quality SVG assets, just the PNG sprites.

I agree, but maybe it is worth double checking? 😬

I'll send an email to Lisa and ask.

@engelgabriel
Copy link
Member

Currently, we use a single sprite, right?
https://open.rocket.chat/packages/emojione_emojione/assets/sprites/emojione.sprites.png

I think that the two versions you have added could be enough, can you push your latest version so I can see on my mbp too?

@engelgabriel engelgabriel added this to the 1.0.0 milestone Mar 20, 2019
@wreiske
Copy link
Contributor Author

wreiske commented Mar 20, 2019

Currently, we use a single sprite, right?
https://open.rocket.chat/packages/emojione_emojione/assets/sprites/emojione.sprites.png

I think that the two versions you have added could be enough, can you push your latest version so I can see on my mbp too?

Pushed. This only has the 64 sprites in it. If you'd like, let me know the two sizes you would like and I'll get them committed. Once I push the other sprites, you'll have to make some code changes for them to display the different sizes.

Change these in /app/emoji-emojione/lib/rocketchat.js

emoji.packages.emojione.emojiSize = 64;
emoji.packages.emojione.spriteSize = 64;

I reached out to Lisa about potentially using the SVG assets, it can't hurt! I'll let you know.

@ggazzo ggazzo changed the title Upgrade EmojiOne to 4.5.0 [WIP] Upgrade EmojiOne to 4.5.0 Mar 20, 2019
@wreiske
Copy link
Contributor Author

wreiske commented Mar 21, 2019

Some things to consider about the SVG sprites...
https://demos.emojione.com/sprites-svg.html

These are not supported in IE8.
IE does not support SVG sprites, to get it working you'll need to include SVG4Everybody

@wreiske
Copy link
Contributor Author

wreiske commented Mar 22, 2019

image

OK.. so this is with the 24x sprite sheet and the 64x sprite sheet without any size modifications. Had to make the reaction buttons a bit bigger.

image

big-reactions2 mov

Are you cool with the slightly bigger reactions?

@engelgabriel
Copy link
Member

@ggazzo I think we can merge this and get to OPEN so people can test this.

@sampaiodiego sampaiodiego requested review from tassoevan and ggazzo May 14, 2019 19:40
@sampaiodiego
Copy link
Member

alright.. this is now ready for merge!!

please @ggazzo and/or @tassoevan (or any other @RocketChat/core ) help reviewing.

thx a ton @wreiske

@wreiske
Copy link
Contributor Author

wreiske commented May 14, 2019

Of course! Thank you for such an AWESOME open source project!

Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

@sampaiodiego, please apply some kind of image compression ;)

app/emoji/client/emojiPicker.js Outdated Show resolved Hide resolved
app/emoji/client/emojiPicker.js Outdated Show resolved Hide resolved
app/emoji/client/emojiPicker.js Outdated Show resolved Hide resolved
app/emoji/client/emojiPicker.js Outdated Show resolved Hide resolved
app/emoji/client/lib/EmojiPicker.js Outdated Show resolved Hide resolved
@ggazzo ggazzo merged commit 68feeb1 into RocketChat:develop May 20, 2019
@wreiske
Copy link
Contributor Author

wreiske commented May 20, 2019

🎉 🚀

@alyssais
Copy link

Reposting my comment from #13686 here in case somebody in this thread can answer:

Is there a copy of the full license that will apply to users of rocketchat after this upgrade? What terms does it impose on redistributors?

If I host a Rocket.Chat instance, or create a package for it, under what conditions am I permitted to use the JoyPixels emoji? Presumably I can't redistribute them freely or modify them, but without any sort of license I can't know whether I'm allowed to use them at all!

@wreiske
Copy link
Contributor Author

wreiske commented May 24, 2019

Reposting my comment from #13686 here in case somebody in this thread can answer:

Is there a copy of the full license that will apply to users of rocketchat after this upgrade? What terms does it impose on redistributors?

If I host a Rocket.Chat instance, or create a package for it, under what conditions am I permitted to use the JoyPixels emoji? Presumably I can't redistribute them freely or modify them, but without any sort of license I can't know whether I'm allowed to use them at all!

I have reached out to my contact on the Joypixels team for comment. I'll let you know her response.

@sampaiodiego sampaiodiego mentioned this pull request May 28, 2019
@wreiske
Copy link
Contributor Author

wreiske commented Jun 13, 2019

@alyssais
Just an update: I have connected Lisa from JoyPixels with Gabriel. They are still working on drafting the agreement for Rocket.Chat's use of JoyPixels. Their licensing department is a bit backed-up, but they should have an amended user agreement to look at next week sometime.

@alyssais
Copy link

alyssais commented Sep 4, 2019 via email

@wreiske
Copy link
Contributor Author

wreiske commented Sep 4, 2019

@alyssais Just an update: I have connected Lisa from JoyPixels with Gabriel. They are still working on drafting the agreement for Rocket.Chat's use of JoyPixels. Their licensing department is a bit backed-up, but they should have an amended user agreement to look at next week sometime.
Guessing you never heard anything?

Actually, they responded to @engelgabriel I believe. Maybe we need to include the license here @engelgabriel ?

@heyyo-droid
Copy link

Reducing the palette to 256 colors, could reduce the sprite size to 1.05MB vs 3.455MB.
people-sprites

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.

EmojiOne version 2 is no longer supported or distributed
9 participants