[PLT-7409] Channel Member List: show status icon and sort by user's status #421

saturninoabril · 2017-12-06T15:43:50Z

Summary

Show status icon and sort by user's status (in the order of online > away > offline > dnd) to the following components:

MemberListChannel
PopoverListMembers (though not specifically mentioned in the ticket)

@jasonblais @esethna You might want to check this out if according to ticket specs. (I'm not really sure about the coverage). Also not sure if this should be for v4.5 as stated in Jira ticket.

Ticket Link

Jira ticket: PLT-7409

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

Ran make check-style to check for style errors (required for all pull requests)
Ran make test to ensure unit and component tests passed
Has UI changes

MusikPolice · 2017-12-06T15:53:34Z

components/member_list_channel/member_list_channel.jsx

 actionUserProps[user.id] = {
 channel: this.props.channel,
 teamMember: teamMembers[user.id],
 channelMember: channelMembers[user.id]
 };
 }
 }
+
+ usersToDisplay.sort((a, b) => {
+ return UserStatusesWeight[a.status] - UserStatusesWeight[b.status] || Utils.sortUsersByDisplayName(a, b);


I don't quite understand this - is it some kind of type coercion trick? It looks like you're subtracting the weighted values of the two users' statuses, and if that results in zero, we know that zero is falsy, so evaluate the OR condition, which sorts by name?

I guess it's short-circuit evaluation. Your subsequent description is what I intend to do which I think works fine as I expected.

MusikPolice · 2017-12-06T15:54:24Z

components/popover_list_members/popover_list_members.jsx

+ const status = UserStore.getStatus(member.id);
+ return {...member, status};
+ }).sort((a, b) => {
+ return UserStatusesWeight[a.status] - UserStatusesWeight[b.status] || Utils.sortUsersByDisplayName(a, b);
 });


Can this sorting be generalized somewhere so that we aren't duplicating the code in the two components?

MusikPolice · 2017-12-06T15:55:19Z

utils/utils.jsx

+ const bName = displayUsernameForUser(b);
+
+ return aName.localeCompare(bName);
+}


Perhaps accept two user objects here, and do all the sorting, including the status weighting, so that the components can just call this function? If you do that, you could easily write unit tests for this function as well, so we'd know that it doesn't break in the future.

saturninoabril · 2017-12-07T11:55:45Z

@MusikPolice updated PR per comments. Also added unit tests.

jwilander · 2017-12-11T13:41:20Z

components/member_list_channel/member_list_channel.jsx

@@ -144,14 +145,18 @@ export default class MemberListChannel extends React.Component {
 const user = users[i];

 if (teamMembers[user.id] && channelMembers[user.id] && user.delete_at === 0) {
- usersToDisplay.push(user);
+ const status = UserStore.getStatus(user.id);


We shouldn't be using anything not in props or state to render. We should pull the statuses when we set the users in state or we should rewrite this component to be more redux friendly and pass it in as props

jwilander · 2017-12-11T13:44:30Z

components/member_list_channel/member_list_channel.jsx

 actionUserProps[user.id] = {
 channel: this.props.channel,
 teamMember: teamMembers[user.id],
 channelMember: channelMembers[user.id]
 };
 }
 }
+
+ usersToDisplay.sort(Utils.sortUsersByStatusAndDisplayName);


We should probably look into moving a bunch of this logic out of the render function if possible. It's possible that we'll render multiple times without the usersToDisplay changing, so rebuilding and sorting, and setting the result to state (or props even) might be worthwhile here. I'm only 1/5 on this though, it's going a bit beyond the scope of the ticket but if a user list is 1000+ in length this could potentially help performance a lot

Turning it into a selector would be even better (though more work)

jwilander · 2017-12-11T13:44:53Z

components/popover_list_members/popover_list_members.jsx

- const bName = Utils.displayUsernameForUser(b);
+ if (this.props.members && teamMembers) {
+ const sortedMembers = this.props.members.map((member) => {
+ const status = UserStore.getStatus(member.id);


Same issue here, needs to be in state or props if it's used for rendering

esethna

@saturninoabril it looks like the status icon component is incorrectly positioned and sized as compared to the center channel or LHS header profile icons:

Other than that it looks good, it will be for 4.6.

saturninoabril · 2017-12-12T17:31:55Z

@jwilander @esethna Thanks! I'll address your comments.

esethna · 2017-12-22T18:35:17Z

@saturninoabril let me know if you have any questions about the items I brought up?

jasonblais · 2018-01-03T01:43:10Z

Hey @saturninoabril, wondering if there's a chance we'd be able to get this in for v4.6? Would be a nice UX improvement.

saturninoabril · 2018-01-04T20:54:17Z

@jasonblais Kindly check whether the comments made by @esethna is satisfied. Also I've made some CSS change that @asaadmahmood might need to take a look.

@jwilander I initially tried to make use of selector but it's taking so long on me. That's why I ended up just moving some of the logics out of render function.

jasonblais · 2018-01-08T15:26:49Z

Thanks! Position looks good.

At the moment, the order is online/away/offline/dnd. Have asked @esethna to confirm whether that's the expected order for DND status.

esethna

DND below offline makes sense since the real time notifications are disabled. Thanks guys!

jwilander

Alright, I'm OK with it since it's not introducing any imports of the flux stores but these components would be much cleaner if they were using redux for everything

jwilander · 2018-01-09T00:55:06Z

@saturninoabril after a rebase it should be good to merge

saturninoabril · 2018-01-09T03:00:26Z

I'll merge this first and then will try to convert the components using redux. Thanks!

* Add typing selector based on channelId and postId * changing getUsersTyping... selector to makeGetUsersTyping...

saturninoabril added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core commiter labels Dec 6, 2017

saturninoabril added this to the v4.6.0 milestone Dec 6, 2017

saturninoabril requested review from MusikPolice and jwilander December 6, 2017 15:43

saturninoabril modified the milestones: v4.6.0, v4.5.0 Dec 6, 2017

MusikPolice reviewed Dec 6, 2017

View reviewed changes

jasonblais assigned esethna Dec 6, 2017

jasonblais added the Setup Old Test Server Triggers the creation of a test server label Dec 6, 2017

jasonblais modified the milestones: v4.5.0, v4.6.0 Dec 6, 2017

saturninoabril removed the Setup Old Test Server Triggers the creation of a test server label Dec 7, 2017

mattermost deleted a comment from mattermod Dec 7, 2017

MusikPolice approved these changes Dec 7, 2017

View reviewed changes

jasonblais added the Setup Old Test Server Triggers the creation of a test server label Dec 11, 2017

jwilander suggested changes Dec 11, 2017

View reviewed changes

esethna suggested changes Dec 12, 2017

View reviewed changes

esethna added the Awaiting Submitter Action Blocked on the author label Dec 12, 2017

jasonblais mentioned this pull request Jan 2, 2018

Update popover_list_members.jsx #1

Closed

1 task

saturninoabril removed the 2: Dev Review Requires review by a core commiter label Jan 4, 2018

mattermost deleted a comment from mattermod Jan 8, 2018

jasonblais added the Setup Old Test Server Triggers the creation of a test server label Jan 8, 2018

esethna added 2: Dev Review Requires review by a core commiter and removed 1: PM Review Requires review by a product manager Setup Old Test Server Triggers the creation of a test server labels Jan 8, 2018

esethna approved these changes Jan 8, 2018

View reviewed changes

jasonblais approved these changes Jan 8, 2018

View reviewed changes

mattermost deleted a comment from mattermod Jan 8, 2018

jasonblais unassigned esethna Jan 8, 2018

jwilander approved these changes Jan 9, 2018

View reviewed changes

jwilander added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Jan 9, 2018

saturninoabril added 3 commits January 9, 2018 10:52

Channel Member List: show status icon and sort by user's status

4a856f1

refactor and add unit tests

c243b9b

move logics out of render function

f4408bb

saturninoabril merged commit 7b02ac5 into mattermost:master Jan 9, 2018

saturninoabril deleted the PLT-7409 branch January 12, 2018 17:25

amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Feb 1, 2018

lindalumitchell added the Tests/Done Release tests have been written label Feb 2, 2018

amyblais added Changelog/Done Required changelog entry has been written and removed Changelog/Done Required changelog entry has been written labels Feb 6, 2018

hmhealey pushed a commit that referenced this pull request Aug 28, 2020

Add typing selector based on channelId and postId (#421)

27446a6

* Add typing selector based on channelId and postId * changing getUsersTyping... selector to makeGetUsersTyping...

hmhealey pushed a commit that referenced this pull request Mar 17, 2021

Add typing selector based on channelId and postId (#421)

c81ebc8

* Add typing selector based on channelId and postId * changing getUsersTyping... selector to makeGetUsersTyping...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PLT-7409] Channel Member List: show status icon and sort by user's status #421

[PLT-7409] Channel Member List: show status icon and sort by user's status #421

saturninoabril commented Dec 6, 2017 •

edited

Loading

MusikPolice Dec 6, 2017

saturninoabril Dec 6, 2017

MusikPolice Dec 6, 2017

MusikPolice Dec 6, 2017 •

edited

Loading

saturninoabril commented Dec 7, 2017

jwilander Dec 11, 2017 •

edited

Loading

jwilander Dec 11, 2017

jwilander Dec 11, 2017

esethna left a comment •

edited

Loading

saturninoabril commented Dec 12, 2017

esethna commented Dec 22, 2017

jasonblais commented Jan 3, 2018

saturninoabril commented Jan 4, 2018

jasonblais commented Jan 8, 2018

esethna left a comment

jwilander left a comment

jwilander commented Jan 9, 2018

saturninoabril commented Jan 9, 2018

[PLT-7409] Channel Member List: show status icon and sort by user's status #421

[PLT-7409] Channel Member List: show status icon and sort by user's status #421

Conversation

saturninoabril commented Dec 6, 2017 • edited Loading

Summary

Ticket Link

Checklist

MusikPolice Dec 6, 2017

Choose a reason for hiding this comment

saturninoabril Dec 6, 2017

Choose a reason for hiding this comment

MusikPolice Dec 6, 2017

Choose a reason for hiding this comment

MusikPolice Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

saturninoabril commented Dec 7, 2017

jwilander Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

jwilander Dec 11, 2017

Choose a reason for hiding this comment

jwilander Dec 11, 2017

Choose a reason for hiding this comment

esethna left a comment • edited Loading

Choose a reason for hiding this comment

saturninoabril commented Dec 12, 2017

esethna commented Dec 22, 2017

jasonblais commented Jan 3, 2018

saturninoabril commented Jan 4, 2018

jasonblais commented Jan 8, 2018

esethna left a comment

Choose a reason for hiding this comment

jwilander left a comment

Choose a reason for hiding this comment

jwilander commented Jan 9, 2018

saturninoabril commented Jan 9, 2018

saturninoabril commented Dec 6, 2017 •

edited

Loading

MusikPolice Dec 6, 2017 •

edited

Loading

jwilander Dec 11, 2017 •

edited

Loading

esethna left a comment •

edited

Loading