Add emoji map button to the compose box.

[ibrand]

• Move the message-control-buttons to the right of the
  compose box in mobile view so that the emoji popover will
  not get cut off on the right side of a mobile screen.
• Append an emoji to the end of the message when a user
  clicks the emoji in the emoji popover.

[smarx]

Automated message from Dropbox CLA bot

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

[timabbott]

I think our style uses a space before the curly-brace here

[ibrand]

@timabbott I fixed up those suggestions. Good thought on having there be a specific case for clicking in the popover vs actually clicking an emoji. It turns out that not handing that was a bug.

[timabbott]

We should probably be fetching this dynamically when we actually are going to access it (i.e. in the call to render_emoji_template). I think the current version will correctly handle the case where we extend emoji_list (maybe it will, but seems clearer regardless to just access this inside render_emoji_template()).

[timabbott]

Added a few more comments on code organization but overall this looks good!

[timabbott]

Also you should add "Fixes: #147." to the commit message so that it'll auto-close the open issue about this feature :)

[ibrand]

Awesome. I will fix up those errors today. Also, I did some user testing, and have determined 2 things:

1. I think the emoji map needs to appear to the left so as not to obstruct the textarea
2. I should take the feature out of a mobile device (small screens) because the emoji window is way too big and most phones also have their own emoji keyboard.

how do you feel about me making those changes as well?

[timabbott]

That sounds reasonable to me!

[ibrand]

@timabbott In response to user testing, I built the emoji pop up into the compose box. That also makes it a lot more responsive for smaller screens and even mobile devices. The design is now finished, let me know what I should fix to prepare for merge

[timabbott]

Awesome @ibrand , I do think this approach is better! Playing around with it, I have two remaining concerns:

(1) When you click the button, it does literally hundreds of GETs to the server; I worry a bit that this will perform poorly if there's any network latency between the user and the server. This is probably OK since they'll all get cached after the first time, and there's probably no easy fix for this.
(2) Would it make sense to make the emoji area either taller or drag-expandable? With only ~2 rows of emoji visible, it feels a bit difficult to look at on my 24" monitor.

@wdaher what do you think of this design look? Screenshot here:

[wdaher]

From a design perspective, 👍 on my end.

Longer-term, I might prefer something more like what Mac OS X does, where there's a quick-access broken-down-by-type selector in the bottom, but this is definitely better than the status quo.

e.g.:

[ibrand]

@wdaher totally. As of right now, I'm pulling emojis straight from the array which is sorted alphabetically, so I suspect putting them into useful categories would be a bit difficult...

@timabbott great! Number 2 (the resizable emoji box) is something I could implement as part of the code that I already wrote. I am hesitant about Number 1. It seems like the solution to the GET requests is to make a spritesheet or something of that variety. I'm not sure where would be the best place to generate the spritesheet, and I also wonder if we should think about the best way to make it useful for other parts of the codebase. Thoughts?

[timabbott]

@ibrand I agree. Re: sorting, I don't think we have the emoji data in a way that's sorted by categories, so that's probably hard at the moment with the current emoji set.

We actually generate the current emoji from a font in tools/emoji_dump/build_emoji (primarily in its helpers) using some tools from the fonttools package; so that would be the right place to put tooling to make a spritesheet...

[timabbott]

@ibrand just checking in on the stauts here -- let me know if there's anything I can do to help move this forward!

[ibrand]

Sorry about not responding sooner! My week's been busy and I didn't have the chance to take a look at the code. My current working strategy for making the box resizable is to try and write code to make a div resizable which has proven slightly harder than I had anticipated.

As for creating a spritesheet. I am going to look at that today with @alliejones. It seems like something that might work well with some of the tool work she's been doing

[timabbott]

OK cool. I just rebased this to https://github.com/timabbott/zulip/tree/tabbott-emoji-map and fixed some lint errors while I was checking this out, in case that's useful for you.

Let me know how that's going and if there's anything I can do to help!

[ibrand]

Awesome! My Vagrant box was having problems for a few days which threw off my work schedule a little, but I should be able to do some work soon.

As of right now, most features are in but in need of repair. In creating the emoji spritesheet some of the older features don't work anymore but I think that's just a CSS issue. I'm also having an issue with CSS escaping in which the "+1" emoji isn't showing up since the "+" is getting escaped.

I'll need to take a look, but I should be able to get around to it soon. Should I pull from your new rebased branch now? How will that work?

[timabbott]

The rebase was fairly simple, so if you've done a moderate amount of stuff on top of your branch you should feel free to just push a new version of your branch and ignore it.

If you'd like to practice your awesome rebase skills to use my version, you can use git remote to add my repository as a second remote and then rebase like this:

git checkout YOURLATESTVERSION
git remote add tabbott https://github.com/timabbott/zulip.git
git fetch tabbott
git rebase --onto tabbott/tabbott-emoji-map REF

Where in this case REF refers to the last of the commits I rebased in your branch's history.

[ibrand]

Hey @timabbott the box resizes (when you use the weird little blue div) and all of the emoji's are on a spritesheet. I've merged in my latest version not because it's done, but because I'm having some difficulty resolving my final bugs. I'm having 2 main problems:

1. I haven't yet figured out how to get an icon to replace that blue box. Ideally it would look like a little handle at corner of the emoji popover window.

2. For some reason, the ".emoji_popover .emoji" click handler doesn't work anymore. Which means that most of the functionality is currently not working. I don't really know why and I was wondering if you'd take a look.

[timabbott]

I'm hoping to find time to take a look this coming week but if I don't I can definitely find time the next week...

[ibrand]

Totally. I'm sorry about progress slowing down a lot. I appreciate your help!

[timabbott]

Yeah, I agree that rename seems likely to be involved in the problem here...

[ibrand]

There looks like there's something even weirder about the emojis on my branch. Have you seen this problem before? They're all oversized and off-center

[timabbott]

Never seen that before! @porterjamesj may have since he modified the emoji system recently.

[timabbott]

(But probably it's a mysterious new effect)

[porterjamesj]

hmmm the emoji images from the new version of the google emoji font that I upgraded us to are bigger than the old ones, but everywhere they're used in the zulip codebase there should be dimensions set on the img tag to make them show up as the appropriate size (the old ones were bigger that they're actually displayed as well).

[porterjamesj]

i think this is the relevant bit.

[timabbott]

Ahh, yeah, that makes a lot of sense.

[timabbott]

@ibrand have you had a chance to take a look at fixing this?

[ibrand]

weekend was swamped. So I haven't looked much yet, @porterjamesj Are you saying that each image tag should have an inline width and height set and that I shouldn't be making adjustments in the css?

Cause it looks like they're huge and off-center even in the emoji sprite sheet before they make it to the page...

[porterjamesj]

hmmmm I'm not sure what you mean by sprite sheet? It looks to me like that CSS should take care of everything provided img tags that represent emoji always have class="emoji"

[alliejones]

Hope it is okay to jump in here! :) I think part of the confusion might be that this PR added a sprite sheet for emoji and James' emoji update was written for the individual emoji images (since the sprite sheet changes haven't been merged yet) and didn't take that into account.

I'm guessing the config for the sprite sheet generator might need to be adjusted (probably somewhere in emoji_dump.py). I'd be willing to take a look at it if that would be helpful.

[timabbott]

Your taking a look would be great @alliejones!

[porterjamesj]

aha! Allie is correct, I hadn't realized that this was adding a sprite sheet.

[ibrand]

Yeah @alliejones that would be totally awesome. I wasn't really sure what to do with the spritesheet code when I saw that the emojis were a weird size.

[alliejones]

This should be SIZE = (136, 136) for the updated emoji.

[alliejones]

I am having Vagrant issues so I can't check how things look in the actual application at the moment, but updating the emoji size fixes the sprite sheet so it looks correct.

[alliejones]

I have fixed my Vagrant problems and it looks like there need to be some stylesheet adjustments made as well. I'll take a look later tonight.

[ibrand]

Is the emoji build script run every time run_dev.py is run?

[alliejones]

I believe it only gets run when you're provisioning.

@ibrand I have a couple commits that fix the emoji sprite on my own fork, do you want me to make a PR against your branch?

[ibrand]

sure! Go for it. I really appreciate the help @alliejones I was a little lost once the emoji build tool got involved

[alliejones]

@ibrand Glad I could help!
However, I can't seem to make a PR against your fork, it doesn't show up in the list of forks for me. @timabbott any thoughts on the best way to get my changes integrated?

[ibrand]

There's always the janky version where you send me a gist of the changes and I just apply them. But that's presumably not the nicest way to do it

[timabbott]

There's a few options you can do with GitHub:
(1) @alliejones can push a branch in her repository, @ibrand can add it as an additional remote and fetch it using e.g.

git remote add alliejones git@github.com:alliejones/zulip.git
git fetch alliejones

(2) @ibrand could give @alliejones write access to her Zulip repository, and then @alliejones could add @ibrand's repository and push to the branch this PR is based on directly.

[timabbott]

FYI this is now being worked on (and has been partially merged) in #1753; closing this in favor of that active PR.

[ibrand]

Wow that is so exciting. Thank you so much for picking this up @umairwaheed

[umairwaheed]

👍

[timabbott]

This was merged a few minutes ago. See #1956 for some follow-up issues. Thanks everyone for your help on this!