RFC: Enhancing users list #850
Conversation
kartikmaji
commented
May 31, 2016
•
kartikmaji
commented
- Star User
- Sort user according to their starred status
- After starring user, updating user list needs a reload
- UI improvements
|
Automated message from Dropbox CLA bot @krtkmj, it looks like you've already signed the Dropbox CLA. Thanks! |
kartikmaji
force-pushed
the
enhancing-users-list
branch
from
47291c1 to
36c5432
Compare
|
@acrefoot could you help @krtkmj with this diff? |
static/js/people.js
Outdated
| @@ -13,6 +13,10 @@ exports.get_by_email = function get_by_email(email) { | |||
| return people_dict.get(email); | |||
| }; | |||
|
|
|||
| exports.get_star_status = function get_star_status(email) { | |||
| return page_params.starred_users.indexOf(email)>=0; | |||
There was a problem hiding this comment.
This causes a test failure when page_params.starred_users is undefined.
|
@neerajwahi Whenever a user stars another user, I update right panel user list. But, changes in user list is reflected only after page is reloaded. I figured out it is because of stale data (where starred is still retaining the old value) applying activity.update_users doesn't reflect any change. So, updated user_info is problem here and guess this is the problem of hybrid state vs normal state. |
kartikmaji
force-pushed
the
enhancing-users-list
branch
from
36c5432 to
3a4705b
Compare
|
@neerajwahi Any suggestion on this UI? Plus, how should it look like if someelse(not starred user) is online, should he/she be on top of the list above starred user? |
|
@neerajwahi I worked around refreshing problem of userlist that I mentioned above. One of the method that I tried is to update page_params in success callback function when user is successfully subscibed/unsubscibed and it worked. Any thoughts? |
kartikmaji
force-pushed
the
enhancing-users-list
branch
4 times, most recently
from
ddd1c9b to
f8592d5
Compare
|
@krtkmj The UI looks pretty good! I'd move the star to the right side of the user name though (so that the names continue to line up vertically) @acrefoot Could you review @krtkmj's code for this diff? |
kartikmaji
force-pushed
the
enhancing-users-list
branch
from
f1104fe to
9d70c8d
Compare
|
@neerajwahi @acrefoot updated this PR. |
| @@ -0,0 +1,20 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
Let's combine this and the 0018 migration files if possible.
|
Okay, I'm done reviewing this round. Thanks for your patience in looking over my many comments. The code is fairly sound--I mostly have comments on style and consistency with the rest of the codebase. The two actually buggy things are the manytomanyfield model being symmetrical and that the UI doesn't seem to update to show "Unstar user ____" when I've already starred a user. Thanks! |
kartikmaji
force-pushed
the
enhancing-users-list
branch
from
ba614c5 to
9d3d768
Compare
|
Updated this PR. |
|
@umairwaheed Could I ask you to take a look at this pull request and give a (non-binding) code review, since this kind of frontend Zulip code is something I gather you're familiar with? |
| <li> | ||
| <a class="star_user"> | ||
| <i class="icon-vector-star"></i> | ||
| {{#tr this}}{{#if starred}}{{t Unstar}}{{else}}{{t Star}}{{/if}} {{t user}} <b>__name__</b>{{/tr}} |
There was a problem hiding this comment.
You can write it like this as well:
{{#tr this}}{{#if starred}}Unstar{{else}}Star{{/if}} user <b>__name__</b>{{/tr}}
There was a problem hiding this comment.
Don't we need to tag keywords Star/Unstar for internationalization?
There was a problem hiding this comment.
I think the point is that both {{#tr this}} and {{t Star}} are ways of tagging for i18n, and we only need one of those....
There was a problem hiding this comment.
Yeah, the block translation tag, {{#tr this }}, should suffice here.
| @@ -21,7 +21,7 @@ | |||
| <li> | |||
| <a class="star_user"> | |||
| <i class="icon-vector-star"></i> | |||
| {{#tr this}}{{#if starred}}{{t Unstar}}{{else}}{{t Star}}{{/if}} {{t user}} <b>__name__</b>{{/tr}} | |||
| {{#tr this}}{{#if starred}}{{t "Unstar"}}{{else}}{{t "Star"}}{{/if}} {{t user}} <b>__name__</b>{{/tr}} | |||
There was a problem hiding this comment.
You can write this line like this as well:
{{#tr this}}{{#if starred}}"Unstar"{{else}}"Star"{{/if}} user <b>__name__</b>{{/tr}}
| @@ -2,7 +2,7 @@ | |||
| {{#each starred_users}} | |||
| {{partial "user_presence_row"}} | |||
| {{/each}} | |||
| <hr> | |||
| <hr/> | |||
| {{#each unstarred_users}} | |||
| {{partial "user_presence_row"}} | |||
| {{/each}} | |||
There was a problem hiding this comment.
This commit can be merged with 681b057
|
@brainwane, sure. @krtkmj, added few comments. |
Split starred and unstarred users with a horizontal line.
kartikmaji
force-pushed
the
enhancing-users-list
branch
from
7056324 to
8f47e15
Compare
|
@umairwaheed Updated this PR. |
|
Cool, the code looks good. |
|
Heads up @krtkmj, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
|
Closing this, since while it's a useful reference for doing this feature, there's enough merge conflicts that we'll probably want to type it in again before merging. Thanks for all your work on this @krtkmj! |
