Add an emoji palette to the compose box by umairwaheed · Pull Request #1753 · zulip/zulip

umairwaheed · 2016-09-05T08:15:58Z

Still need to test this.

smarx · 2016-09-05T08:15:59Z

Automated message from Dropbox CLA bot

@umairwaheed, it looks like you've already signed the Dropbox CLA. Thanks!

umairwaheed · 2016-09-05T11:41:49Z

Works now. The only problem is that when compose box is opened against a particular topic and Emoji icon is clicked, the map doesn't stay visible.

umairwaheed · 2016-09-06T09:59:31Z

@timabbott, please review.

timabbott · 2016-09-08T03:39:38Z

Looks like the test suite doesn't pass? I just triggered a rerun of the tests to see for sure.

timabbott · 2016-09-08T03:39:56Z

(Also, from a commits perspective, are you imagining we'll squash these together for merging?)

umairwaheed · 2016-09-08T10:10:22Z

Using glue from this PR, jorgebastida/glue#211, as we need Jinja2==2.8.

umairwaheed · 2016-09-08T10:39:33Z

@timabbott, tests pass now.

Also, from a commits perspective, are you imagining we'll squash these together for merging?

You want me to merge these commits?

timabbott · 2016-09-08T22:26:44Z

OK, I merged all the emoji dump changes (after fixing a few bugs). I think we should squash the frontend changes into one commit since they're now just bug fixes on the main commit (I've done that in the tabbott/emoji-map branch if you want to pick up from there). Additionally, we need to address the following:

Opening the emoji pallet box takes like 5s, which is way too slow -- we should profile and debug.
Now that we're generating the sprite sheet which takes like 1m to run, to avoid slowing down provisioning, we need to add some sort of caching similar to what we've been doing for venv/npm stuff to make this not a performance problem for the provisioning process.
The JS for managing the compose box should auto-close the emoji panel when the compose box is closed, so that it isn't open when one re-opens the compose box.
The actual sprite sheet display isn't totally working for me

umairwaheed · 2016-09-09T11:45:56Z

The actual sprite sheet display isn't totally working for me

You mean design wise or there is some bug?

timabbott · 2016-09-09T14:38:22Z

I mean an actual bug -- the emoji are clickable in the sprite sheet display but invisible.

(I guess you should rebase regardless to drop the backend commits).

umairwaheed · 2016-09-14T11:28:28Z

working on this

umairwaheed · 2016-09-15T11:20:37Z

I mean an actual bug -- the emoji are clickable in the sprite sheet display but invisible.

@timabbott, sprite.css has classes of the type 'emoji-glyph00005' whereas we use classes which are like 'emoji-angry' in our templates, I wonder if this is the problem. I couldn't find where 'emoji-angry' css class was declared in our code.

umairwaheed · 2016-09-16T07:09:49Z

The actual sprite sheet display isn't totally working for me

@timabbott, this is fixed now.

umairwaheed · 2016-09-16T09:53:41Z

Opening the emoji pallet box takes like 5s, which is way too slow -- we should profile and debug.

Most likely due to the size of 'sprite.png', it is 1.5MB.

umairwaheed · 2016-09-16T10:18:55Z

The JS for managing the compose box should auto-close the emoji panel when the compose box is closed, so that it isn't open when one re-opens the compose box.

Done

umairwaheed · 2016-09-20T10:57:56Z

working on this:

Now that we're generating the sprite sheet which takes like 1m to run, to avoid slowing down provisioning, we need to add some sort of caching similar to what we've been doing for venv/npm stuff to make this not a performance problem for the provisioning process

umairwaheed · 2016-09-23T11:26:47Z

@timabbott, caching is done!

timabbott · 2016-09-23T18:31:10Z

awesome! I'll try to find time to review this soon. I'm still thinking about what we can do about the 1.5MB download issue... it might be worth writing a bit of code to download it in the background once the webapp is idle if it isn't cached already, to make the loading delay happen when the user isn't looking.

umairwaheed · 2016-09-26T10:27:08Z

@timabbott, now the code retrieves the sprite image in the background as well.

brainwane · 2016-09-26T19:32:10Z

ccing @brockwhittaker - You might want to take a look at this PR, from a frontend/design perspective.

timabbott · 2016-09-27T05:51:52Z

    <div class='notifications top-right'></div>
 </div>
+<!--This is just to retrieve the sprite image from the server and cache it-->
+<div class="emoji emoji-100" style='margin-left: -1000'></div>


Hmm. Is there a way we can do this so that the network traffic is deferred until after the page is loaded, and it doesn't take up space in the DOM?

We can add this element, <div class="emoji emoji-100" style='margin-left: -1000'></div>, through javascript after the page has loaded.

Done. The image is now cached using Javascript.

umairwaheed · 2016-09-27T08:01:34Z

@timabbott, done!

timabbott · 2016-09-30T17:48:17Z

I merged a couple of the simpler backend commits, but I think the emoji dump caching change probably needs work to interact properly with the update-prod-static tooling for building production release tarballs, right? I think as-is it'll just pack up a symlink that will unpack as a broken symlink.

Also, I think we should include the rest of tools/setup/emoji_dump/* in the files hashed in get_success_stamp, since we want to regenerate the emoji if the code changes as well...

Otherwise, the caching code looks good! The speed improvement to provisioning is pretty noticable!

umairwaheed · 2016-10-03T12:15:46Z

sure. working on this.

umairwaheed · 2016-10-03T13:05:52Z

but I think the emoji dump caching change probably needs work to interact properly with the update-prod-static tooling for building production release tarballs, right? I think as-is it'll just pack up a symlink that will unpack as a broken symlink.

@timabbott, the release tarball comes out correctly. There are no symlinks.

- Expand a box full of emojis into the compose window for users to graphically select emojis. - Append an emoji to the end of the message when a user clicks the emoji in the emoji box. - Trap the escape key to always close the emoji box before closing anything else if the box is open. - Fixes: zulip#147.

`glue` ingores the + sign in the name of the emoji and creates the css class without it. This causes the emoji pallete to miss '+1' emoji. This commit creates a translation map from emoji name to css class and uses that to create the emoji popover.

umairwaheed · 2016-10-04T06:27:31Z

Also, I think we should include the rest of tools/setup/emoji_dump/* in the files hashed in get_success_stamp, since we want to regenerate the emoji if the code changes as well...

@timabbott done.

timabbott · 2016-10-10T23:18:36Z

Merged, thanks @umairwaheed for getting this finished! (Also, huge thanks to @ibrand for writing the first version of this!).

There's still some things that we should improve soon, which I've opened as #1956.

umairwaheed changed the title ~~[WIP] Add an emoji palette to the compose box~~ Add an emoji palette to the compose box Sep 5, 2016

umairwaheed force-pushed the 147-emoji-palette branch from 96c8e1b to c0249f2 Compare September 8, 2016 07:37

umairwaheed force-pushed the 147-emoji-palette branch 2 times, most recently from d98e4cd to b122341 Compare September 8, 2016 10:38

timabbott mentioned this pull request Sep 8, 2016

Add emoji map button to the compose box. #191

Closed

umairwaheed force-pushed the 147-emoji-palette branch from b122341 to cc5f5c1 Compare September 16, 2016 07:06

umairwaheed force-pushed the 147-emoji-palette branch from cc5f5c1 to a7fcd11 Compare September 16, 2016 09:52

umairwaheed force-pushed the 147-emoji-palette branch 2 times, most recently from 569e894 to 4820d2e Compare September 23, 2016 11:14

umairwaheed force-pushed the 147-emoji-palette branch 2 times, most recently from c31c7f3 to 95cd782 Compare September 26, 2016 10:11

timabbott reviewed Sep 27, 2016

View reviewed changes

umairwaheed force-pushed the 147-emoji-palette branch from 95cd782 to 2a737a0 Compare September 27, 2016 07:58

umairwaheed force-pushed the 147-emoji-palette branch from 2a737a0 to 53af510 Compare October 3, 2016 13:04

ibrand and others added 5 commits October 4, 2016 10:35

Close emoji panel when compose box is closed.

b11b224

Cache emoji dump.

cfe616f

Retrieve the sprite image in the background.

ce305f8

umairwaheed force-pushed the 147-emoji-palette branch from 53af510 to ce305f8 Compare October 4, 2016 06:25

timabbott closed this Oct 10, 2016

umairwaheed deleted the 147-emoji-palette branch August 14, 2017 04:43

Uh oh!

Uh oh!

Conversation

umairwaheed commented Sep 5, 2016

Uh oh!

smarx commented Sep 5, 2016

Uh oh!

umairwaheed commented Sep 5, 2016

Uh oh!

umairwaheed commented Sep 6, 2016

Uh oh!

timabbott commented Sep 8, 2016

Uh oh!

timabbott commented Sep 8, 2016

Uh oh!

umairwaheed commented Sep 8, 2016

Uh oh!

umairwaheed commented Sep 8, 2016

Uh oh!

timabbott commented Sep 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

umairwaheed commented Sep 9, 2016

Uh oh!

timabbott commented Sep 9, 2016

Uh oh!

umairwaheed commented Sep 14, 2016

Uh oh!

umairwaheed commented Sep 15, 2016

Uh oh!

umairwaheed commented Sep 16, 2016

Uh oh!

umairwaheed commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

umairwaheed commented Sep 16, 2016

Uh oh!

umairwaheed commented Sep 20, 2016

Uh oh!

umairwaheed commented Sep 23, 2016

Uh oh!

timabbott commented Sep 23, 2016

Uh oh!

umairwaheed commented Sep 26, 2016

Uh oh!

brainwane commented Sep 26, 2016

Uh oh!

timabbott Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

umairwaheed Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

umairwaheed Sep 27, 2016

Choose a reason for hiding this comment

Uh oh!

umairwaheed commented Sep 27, 2016

Uh oh!

timabbott commented Sep 30, 2016

Uh oh!

umairwaheed commented Oct 3, 2016

Uh oh!

umairwaheed commented Oct 3, 2016

Uh oh!

umairwaheed commented Oct 4, 2016

Uh oh!

timabbott commented Oct 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

timabbott commented Sep 8, 2016 •

edited

Loading

umairwaheed commented Sep 16, 2016 •

edited

Loading