[wip] popovers: rendering the emoji picker only once. #4301

tejaskasetty · 2017-03-25T14:40:31Z

Fix to #4300
The rendering of emoji for the emoji picker is made only once when the emoji Picker UI is first used.
Rest of the requests just toggles the already rendered emoji picker.

Notable changes - Earlier when ever the emoji picker was closed the focus on the compose text box would be removed. In this commit, when the emoji picker is closed the compose text box remains in focus. And this happens also when we open the emoji picker. This change will be reverted back if not needed.

Observations - There are other areas like this if corrected can improve the efficiency of the emoji picker UI on the frontend. I will look for them and work on improving them.

smarx · 2017-03-25T14:40:33Z

Automated message from Dropbox CLA bot

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

tejaskasetty · 2017-03-25T14:44:34Z

This also would be a way for progress on #1956 . Which deals with improvement in performance of emoji picker, which I have been working on parallely.

showell · 2017-03-25T16:16:33Z

Hi @tejaskasetty, can you fix the commit messages to conform to our guidelines here?:

https://zulip.readthedocs.io/en/latest/version-control.html#commit-messages

tejaskasetty · 2017-03-25T16:57:43Z

yeah sure.

tejaskasetty · 2017-03-25T17:56:25Z

The thing is, I would like to just submit one proper commit. The multiple commits that i submitted were because it initially didn't pass the Travis CI build. How can i do that? I m really sorry, I should have read the commit discipline. Now i know how to write a proper commit message. And also I m still getting familiarized with git.

showell · 2017-03-25T18:12:29Z

@tejaskasetty, It is very common for contributors to have issues understanding git, so please don't worry, we're here to help. In order to squash your commits into one proper commit, you will want to learn how to do an interactive rebase. If you go on to the "development help" stream on chat, there should be multiple folks that can walk you through this, although expect some delay for help during the weekend.

adnrs96 · 2017-03-25T18:12:52Z

You can squash all the commit's into one using an interactive rebase in git.
Reading this might be useful.
Also Zulip has a git guide here.

zulipbot · 2017-03-25T22:31:49Z

Hello @zulip/server-emoji members, this pull request references an issue with the "area: emoji" label, so you may want to check it out!

tejaskasetty · 2017-03-25T22:38:44Z

Thanks a lot. It was very informative. Got to learn more about Git. By the way, how is the updated commit. Is this according to the guidelines?

showell · 2017-03-26T16:00:14Z

static/js/popovers.js

@@ -519,10 +521,14 @@ exports.register_click_handlers = function () {
        if (emoji_map_is_open) {
            // If the popover is already shown, clicking again should toggle it.
            popovers.hide_emoji_map_popover();
-            return;
+        } else { // popovers.hide_all();


I think you want to remove the above comment.

Yeah. I kept it so that it would remind me to check areas like this, where there has been unwanted calls to hide all popovers. so they I could understand them. so that i could optimize other areas like this as well. I forgot to remove it later.

I don't think that it is an unwanted call. Purpose of this call is to hide other popovers like message actions popover, if they are open before opening the emoji picker popover. I think you should add it back.

showell · 2017-03-26T16:04:39Z

The code here looks pretty straightforward, and I appreciate the detail of the commit message.

I'd like to reword the first line of the commit message a bit. Since we have two different emoji pickers, and I believe this is for the compose box, say this: compose: Render the emoji picker only once.

HarshitOnGitHub

@tejaskasetty It is looking good but I think it is still not complete yet. You need to re-render the emoji pickers in case a realm emoji is added/deleted as pointer out by Tim! :)

tejaskasetty · 2017-03-27T06:55:25Z

@HarshitOnGitHub yeah you are right. I will add that case and submit.

tejaskasetty · 2017-03-27T07:01:11Z

@showell. I ll change commit header. Actually, I m also working on unifying the two different emoji pickers as well. I m going through the code base to get the ideas of all the places where this occurs. Going through the code base is taking a bit of time. Once i cover all the related files, i will start of with that too.

showell · 2017-03-27T15:14:03Z

I added "wip" to the PR title to indicate this is still a work in progress.

tejaskasetty · 2017-03-28T03:04:29Z

okay

timabbott · 2017-04-06T01:07:01Z

@tejaskasetty can you resolve the outstanding feedback and let us know when this is ready for review?

tejaskasetty · 2017-04-07T06:43:29Z

I m really sorry, for the delay. I was checking through the server side code for - "How the case of updating a realm_emoji was handled. Took some time jumping between various files trying to understand the server part. Later realized that, whenever a realm_emoji was added, in server_events.js

case 'realm_emoji':
        emoji.update_emojis(event.realm_emoji);
        admin.populate_emoji(event.realm_emoji);
        break;

this case would be triggered. So, i didn't have to change/add anything on the server side.
I just added a function call in the above case to reset the emoji_is_rendered in popovers to false. So, the emojis are rendered again when the emoji set is updated.

tejaskasetty · 2017-04-07T06:45:09Z

Sorry accidently closed it

tejaskasetty · 2017-04-07T07:13:22Z

Coming back to the issue, the mistake was i thought it was not handled on the server side. So, simply went through a lot of server side code.
After adding the fix in server_events.js, I tested it by adding custom emoji's using the administrator account and checked the working. Whenever the new emoji is added, the the emoij set will be updated and rendered again.

showell · 2017-04-07T14:31:29Z

It looks like you need to fix the node test for dispatch.js, which should be a one-line fix, but I'm not sure.

You can run ./tools/test-js-with-node to reproduce.

I'm not sure about the other Travis errors--they may have been flakes unrelated to your changes.

tejaskasetty · 2017-04-10T02:14:56Z

@showell I have made the correction in emoji.update_emojis to support deletion of emoji and added the popovers.js dependecy in dispatch.js and the tests seems to pass now . Please review.

HarshitOnGitHub

Left a few comments and please have proper capitalization and punctuation in your commit messages, otherwise looking good to me. :)

HarshitOnGitHub · 2017-04-11T11:43:30Z

static/js/popovers.js

@@ -380,6 +380,10 @@ function user_sidebar_popped() {
    return current_user_sidebar_popover !== undefined;
 }

+exports.reset_emoji_map_is_rendered = function () {


Minor but I would call it something like reset_emoji_popover().

HarshitOnGitHub · 2017-04-11T11:46:41Z

static/js/emoji.js

@@ -29,6 +29,7 @@ exports.emoji_name_to_css_class = function (emoji_name) {

 exports.update_emojis = function update_emojis(realm_emojis) {
    // Copy the default emoji list and add realm-specific emoji to it
+    exports.realm_emojis = {};


You can add a comment here explaining why this is needed.

tejaskasetty · 2017-04-11T16:43:37Z

@HarshitOnGitHub Thanks, for the review. I made the changes. And updated the commit messages. I'll be careful about these aspects in future commits.

* Whenever the emoji picker is opened a call is made to render the emoji's. This rendering happend everytime the emoji picker was opened. Thus, resulting in duplicates of emoji's getting appended in the emoji picker over multiple open and close. * This commit, is a fix to render the emoji's only once when the emoji picker is opened for the first time. Further calls just toggles the emoji picker showing the already rendered emoji's. This enhances the performance of Emoji picker considerably because there is no overhead of making a request to get the emoji's from the server, each time the emoji picker is opened. * Other changes -- on closing the emoji picker, the compose box remains in focus. Fixes: #4300. See Also: #3952.

adnrs96 · 2017-04-15T06:06:56Z

@tejaskasetty It seems from the last commit that you used the merge branches workflow whereas Zulip prefers rebase. Would you mind rebasing your branch instead of doing a merge.
You might find this useful.

tejaskasetty · 2017-04-15T06:17:35Z

yeah I realized that. So, finished rebasing will push it to my remote branch now.

* reset the emoji popover in case of an event regarding update of realm_emoji. * test-node-with-js: Add dependency - popovers module; In dispath.js to support popovers object.

timabbott · 2017-04-18T19:22:26Z

Nice, this version is really clean. Merged, thanks @tejaskasetty!

zulipbot added the needs review label Mar 25, 2017

zulipbot added needs more work and removed needs review labels Mar 26, 2017

zulipbot assigned showell Mar 26, 2017

showell reviewed Mar 26, 2017

View reviewed changes

HarshitOnGitHub reviewed Mar 26, 2017

View reviewed changes

zulipbot added needs review needs more work and removed needs more work needs review labels Mar 27, 2017

zulipbot assigned HarshitOnGitHub Mar 27, 2017

showell changed the title ~~popovers: rendering the emoji picker only once.~~ [wip] popovers: rendering the emoji picker only once. Mar 27, 2017

zulipbot added needs review and removed needs more work labels Apr 7, 2017

tejaskasetty closed this Apr 7, 2017

tejaskasetty reopened this Apr 7, 2017

HarshitOnGitHub reviewed Apr 11, 2017

View reviewed changes

zulipbot added needs more work needs review and removed needs review needs more work labels Apr 11, 2017

tejaskasetty mentioned this pull request Apr 15, 2017

Emoji Picker is unnecessarily rendering the emojis everytime. #4300

Closed

tejaskasetty added 2 commits April 15, 2017 12:24

compose: Re-render emoji picker when realm_emoji is added/deleted.

302b4a3

* reset the emoji popover in case of an event regarding update of realm_emoji. * test-node-with-js: Add dependency - popovers module; In dispath.js to support popovers object.

update_emojis: Handle the case involving deletion of realm emoji.

fc2fcd4

timabbott closed this Apr 18, 2017

zulipbot removed the needs review label Apr 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] popovers: rendering the emoji picker only once. #4301

[wip] popovers: rendering the emoji picker only once. #4301

tejaskasetty commented Mar 25, 2017

smarx commented Mar 25, 2017

tejaskasetty commented Mar 25, 2017 •

edited

showell commented Mar 25, 2017

tejaskasetty commented Mar 25, 2017

tejaskasetty commented Mar 25, 2017

showell commented Mar 25, 2017

adnrs96 commented Mar 25, 2017

zulipbot commented Mar 25, 2017

tejaskasetty commented Mar 25, 2017 •

edited

showell Mar 26, 2017

tejaskasetty Mar 27, 2017 •

edited

HarshitOnGitHub Mar 27, 2017

showell commented Mar 26, 2017

HarshitOnGitHub left a comment

tejaskasetty commented Mar 27, 2017 •

edited

tejaskasetty commented Mar 27, 2017

showell commented Mar 27, 2017

tejaskasetty commented Mar 28, 2017

timabbott commented Apr 6, 2017

tejaskasetty commented Apr 7, 2017 •

edited

tejaskasetty commented Apr 7, 2017

tejaskasetty commented Apr 7, 2017 •

edited

showell commented Apr 7, 2017

tejaskasetty commented Apr 10, 2017 •

edited

HarshitOnGitHub left a comment

HarshitOnGitHub Apr 11, 2017

HarshitOnGitHub Apr 11, 2017

tejaskasetty commented Apr 11, 2017

adnrs96 commented Apr 15, 2017

tejaskasetty commented Apr 15, 2017

timabbott commented Apr 18, 2017

[wip] popovers: rendering the emoji picker only once. #4301

[wip] popovers: rendering the emoji picker only once. #4301

Conversation

tejaskasetty commented Mar 25, 2017

smarx commented Mar 25, 2017

tejaskasetty commented Mar 25, 2017 • edited

showell commented Mar 25, 2017

tejaskasetty commented Mar 25, 2017

tejaskasetty commented Mar 25, 2017

showell commented Mar 25, 2017

adnrs96 commented Mar 25, 2017

zulipbot commented Mar 25, 2017

tejaskasetty commented Mar 25, 2017 • edited

showell Mar 26, 2017

Choose a reason for hiding this comment

tejaskasetty Mar 27, 2017 • edited

Choose a reason for hiding this comment

HarshitOnGitHub Mar 27, 2017

Choose a reason for hiding this comment

showell commented Mar 26, 2017

HarshitOnGitHub left a comment

Choose a reason for hiding this comment

tejaskasetty commented Mar 27, 2017 • edited

tejaskasetty commented Mar 27, 2017

showell commented Mar 27, 2017

tejaskasetty commented Mar 28, 2017

timabbott commented Apr 6, 2017

tejaskasetty commented Apr 7, 2017 • edited

tejaskasetty commented Apr 7, 2017

tejaskasetty commented Apr 7, 2017 • edited

showell commented Apr 7, 2017

tejaskasetty commented Apr 10, 2017 • edited

HarshitOnGitHub left a comment

Choose a reason for hiding this comment

HarshitOnGitHub Apr 11, 2017

Choose a reason for hiding this comment

HarshitOnGitHub Apr 11, 2017

Choose a reason for hiding this comment

tejaskasetty commented Apr 11, 2017

adnrs96 commented Apr 15, 2017

tejaskasetty commented Apr 15, 2017

timabbott commented Apr 18, 2017

tejaskasetty commented Mar 25, 2017 •

edited

tejaskasetty commented Mar 25, 2017 •

edited

tejaskasetty Mar 27, 2017 •

edited

tejaskasetty commented Mar 27, 2017 •

edited

tejaskasetty commented Apr 7, 2017 •

edited

tejaskasetty commented Apr 7, 2017 •

edited

tejaskasetty commented Apr 10, 2017 •

edited