-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Conversation
Added user status to channel member list
Added CSS to fix the online indicator positioning and size
@@ -124,9 +124,9 @@ export default class PopoverListMembers extends React.Component { | |||
key={'popover-member-' + i} | |||
> | |||
<ProfilePicture | |||
className='user__picture' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this prop does anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that prop exists on the portrait at the top left corner so I've added it, perhaps it will be added later so it will keep the same format. If you think it won't I can remove it, I was merely following the already existing patterns.
className='user__picture' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's supported by the img
tag used there, but it doesn't do anything when you're using a ProfilePicture
component. We should remove it since it might suggest to people using this code that this component will be affected by any CSS changes made to user__picture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, removed.
src={Client4.getProfilePictureUrl(m.id, m.last_picture_update)} | ||
width='40' | ||
height='40' | ||
status={UserStore.getStatus(m.id)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of contacting the store directly, can you pass this in from the container? You can use the getStatusForUserId
selector from mattermost-redux/selectors/entities/users
to get it from the redux store instead of using the old UserStore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was using the store because all the status example around the code were also using the store. I am not sure if I did it right as I am not knowledge in redux, but let me know, if the syntax is right.
src={Client4.getProfilePictureUrl(m.id, m.last_picture_update)} | ||
width='40' | ||
height='40' | ||
status={getStatusForUserId(state, m.id)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad for not clarifying. I wasn't sure if you'd worked with Redux before, so I can give you a quick rundown of how it should work.
Basically, we want the container object (components/popover_list_members/index.js
) to be the only thing that talks directly to the store. It will then pass anything that this component needs to render as part of the props. If you look at the mapStateToProps
function in that file, it's used to read from the redux store and set the props for this component. So what you'll want to do is:
- Add status to the object returned by
mapStateToProps
so that it gets passed in asthis.props.status
- Add the prop type as you would with any other prop
- Use
this.props.status
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to explain that, very helpful, hopefully it looks alright now :)
Changed to use store thru redux
Added statuses to props
Awesome, looks good |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was one minor theme issue, fixed that. Other than that looks good.
@prixone if you want to give this a rebase we can get it merged |
@prixone Would you be able to help resolve the merge conflicts and rebase the PR? Should be good to merge otherwise |
Closing in favor of #421, which should cover the changes made here |
* MM-8589: change i18n/getLanguages to use store This also removes the caching of the available languages data structure, as it obviously needs to be able to change. It's not on a frequently used code path so the caching appears to (at least now) be unnecessary. * MM-8589: eliminate UserStore.getNoAccounts() No changes to the use of global variables here. Rather, I'm just pushing down an indirect use of same. * MM-8589: expunge global mm_(config|license) from claim components This also eliminates `LdapPasswordFieldName` as the server-side configuration does not exist. * MM-8589: clarify props passed to claim components * MM-8589: expunge unnecessary ownProps * MM-8589: use the get(Config|License) selector
) (#862) * MM-9635: expunge global mm_config from profile popover component. This change updates the tests to expect a connected profile popover, which in turns subtly changes the pluggable expectations. Tested with the hovercardexample plugin to verify that the connected ProfilePopover can still be overridden. * MM-9635: expunge global mm_config from error page * MM-9635: expunge global mm_config from team selection page * MM-9635: expunge global mm_config from switch channel provider
add different colors to different threads
Release branch for kredily changes
Ts migrate test
Summary
Added user status to channel member list
Added CSS to fix the online indicator positioning and size
Ticket Link
PLT-7409 item 1 only, which I had laying around on my own build of Mattermost.
Can't rebase, reference mattermost/mattermost#7382
Checklist