[MM-24027] Add profile pop-over when clicking a user in the View/Manage members modals

[shred86]

Summary

Added the ability to view user profile pop-over when clicking the profile picture or username from the view/manage members modal. Mimics the behavior when clicking a profile picture or username from a post.

Tested on Chrome 81.0.4044.92, Safari 13.1 (15609.1.20.111.8) and the Mattermost web app 4.4.0 (4.4.0.5682).

Ticket Link

JIRA ticket: MM-24027
Fixes mattermost/mattermost#14266

Screenshots

[mattermod]

Test server destroyed

[mattermod]

Creating a new SpinWick test server using Mattermost Cloud.

[mattermod]

Mattermost test server created! 🎉

Access here: https://mattermost-webapp-pr-5302.test.mattermost.cloud

Account Type	Username	Password
Admin	sysadmin	Sys@dmin123
User	user-1	User-1@123

[shred86]

One issue I'll need to fix, the - is showing up when a user doesn't have a name or nickname specified.

Edit: Just committed a fix for this. I'm using   for the spacing because for whatever reason, when I try to use spaces next to the string ' - ', the blank spaces get truncated. I'm assuming it has something to do with the style this row is wrapped in, but I noticed in other components that   is also used and I'm assuming it's for this same reason.

[esethna]

@shred86 sweet! One issue I found is that clicking "Send Message" in the profile pop-over should clode the view/manage members modal

[shred86]

@shred86 sweet! One issue I found is that clicking "Send Message" in the profile pop-over should clode the view/manage members modal

@esethna Hm, I'm trying to duplicate this but not sure how. If I click "Send Message" from the pop-over profile, it opens a direct message window thus "closing" everything.

Did you perhaps mean the "Add to channel" option? If so, would the desired behavior be to close the view/manage member modal? I guess I could see it being useful to keep open, but I'll obviously try to figure out how to close it if that's not the desired behavior. :)

Edit: This also applies for "Edit Account Settings", definitely want to close the view/manage members modal here.... or do we? :)

[esethna]

@esethna Hm, I'm trying to duplicate this but not sure how. If I click "Send Message" from the pop-over profile, it opens a direct message window thus "closing" everything.

Here's an example of the repro @shred86 :

Thanks for pointing out those other cases. Given you are taking action on another modal in the case of "Account Settings" and "Add to channel", we should avoid having modals over modals, hence closing the original view members modal when those actions are taken would be the expected UX

[shred86]

Ah, yeah that's not good! That's strange I'm not seeing the same behavior in Safari or Chrome. I am trying to figure out how to close the View/Manage Members modal though.

[shred86]

Some additional issues identified and fixed: the hasMentionprop wasn't being passed for UserProfile or ProfilePicture thus making the @username not a clickable link to populate the message bar. Yet another case the View/Manage Members modal will need to be closed...

[shred86]

This is the best solution I can come up with right now for being able to close the View/Manage Members modal. I'm passing the handleExit method as a prop from ChannelMembersModal down the component tree to the ProfilePopover component. From here, it's a simple check to see if the prop that I passed down exists (which will only exist if the profile popover is accessed from the View/Manage Members modal), then call handleExit to close the View/Manage Members modal. This check is obviously required since the profile popover can be accessed from main channel message windows when clicking a username.

There is one known issue I haven't quite figured out but when you load up the web app for the first time (i.e. refresh the browser), the first time you select "Send Message" from the popover profile when accessed from the View/Manage Members modal, it throws this error:

react-dom.development.js:506 Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.

It only occurs the first time when clicking "Send Message" and only from the popover profile when accessed from the View/Manage Members modal.

Edit: Just realized this is also a factor from TeamMembersModal so I created a handleExit method and passed it down as a prop. Fortunately, it shares a lot of the same components as ChannelMembersModal. Just need to make sure there's no other similar modals.

[shred86]

After doing some more investigating, I think I'm understanding this a bit better and where the potential issue is. How I'm currently closing the View/Manage Team and Members modal works (passing handleExit prop through many different child components), but I think a potentially better way is using closeModal in actions/views/modals. However, there is a slight "issue" with how the View/Manage Team and Members modal is being created. When it's opened via the menu or clicking the channel name, that occurs through the MainMenu and ChannelHeaderDropdown components which uses the Menu.ItemToggleModalRedux component. This results in the openModal action being called which saves the modal ID and state to redux and allows us to access and close the modals using closeModal from any component.

When the View/Manage Team and Members modal is accessed through the PopoverListMembers component (small person icon on top right of the screen), all of the states for modals opened this way are stored in this component. The modals are being created directly (e.g. importing the ChannelMembersModal component directly), thus the openModal action is never called and the model ID and state is not stored using redux, meaning closeModal can't be utilized.

So I see two options:

1. Keep the current implementation and just pass the handleExit prop down the component tree as I'm doing currently.
2. Restructure PopoverListMembers so it's calling the openModal action to store the modal ID and state in redux. This way we can close the modals using closeModal.

The second option seems "cleaner", but one issue still remains which is I would have to call closeModal on both the View/Manage Team and Members modals, which would result in closeModal being called on a modal that doesn't exist. The obvious work around is check where it's being called from, but that would still require passing a prop down the component tree or perhaps we can just add a check in closeModal to only dispatch the action if the modal ID exists and a certain state condition is true.

Would be nice if there was a dismissAllModals method like there is with React Native Navigation. :)

[shred86]

Major changes made with the last commit:

• When ChannelMembersModal is accessed from PopoverListMembers, it now opens it using openModal which allows us to now close the modal using closeModal. With these changes, I've removed the passing of props down several children components to close the View/Manage Members modal.
• Added a new method to ProfilePopover called handleCloseModals which checks if the modal exists in redux. If it does, it then checks the open property and if it's true, closeModal is called when "leaving" the modal.
• Minor refactoring of some code and clean up between ChannelMembersModal and TeamMembersModal.

There is a lot of commented out code in popover_list_members.jsx for ease of review. Some of it is based on the new handleCloseModals method but most of it is code that I cannot figure out its purpose and was hoping to get some feedback. I've spent a couple hours going over every scenario I could think of and I can't figure out why or when the Team Members and Channel Invite modals would need to be opened or closed from the PopoverListMembers component.

I'm holding off on updating the tests for now since depending on whether or not I'm able to remove the code mentioned above in popover_list_members.jsx will determine if some changes to the tests need to be made.

Edit: I’ll also take care of PR #13469 with this PR since I’m working on the popover_list_members module anyways.

[bradjcoughlin]

@shred86 I would suggest removing the comments because as a PR reviewer I can see the removed lines. The comments just end up looking like new lines in the diff 😃

[shred86]

@bradjcoughlin Makes sense - I probably shouldn't have even made the commit yet. I basically wanted to know if those commented out functions are even used and didn't want to delete them quite yet since I wasn't sure, but it's easy enough to copy and paste them back in! :)

I can make a new commit with the commented stuff removed.

[jgilliam17]

Thank you @shred86
Tested, looks good to merge.

• Verified users are able to view user profile pop-over when clicking the profile picture on manage members modal. same behavior as when clicking on a profile picture or username from a post.
• Verified all links on the popover work as expected (send message, add to channel, email, @ mention and edit acct. settings for current user)

[jgilliam17]

@shred86
I noticed during PR testing, but thought it was out of scope here and an issue on master.
Confirmed it is not broken on master.test, with help from @ogi-m.
Can you please check if this PR is breaking the guest label, appears doubled on the manage member modal?
Thank you.

[shred86]

@jgilliam17 Great catch! I found the issue. Since we're using the UserProfile component now to display the username which includes the bot and guest badges. I've removed the redundant bot and guest badge in UserListRow. I can't test this locally since I only have the teams edition (no guest accounts), but we should be able to checkk it out once SpinWick updates.

[mattermod]

New commit detected. SpinWick will upgrade if the updated docker image is available.

[mattermod]

Test server creation failed. See the logs for more information.

[mattermod]

Mattermost test server updated with git commit 982c5333137a4f961146797e5158f7584f18ca3e.

Access here: https://mattermost-webapp-pr-5302.test.mattermost.cloud

[jgilliam17]

Thank you @shred86 for a quick fix and thank you for adding bots to the list, I totally missed that.
Re-tested - looks good.

[bradjcoughlin]

I also poked around at the modal code and came to the same conclusion as @hmhealey (that it's old and needed to be removed). Everything thing else looks good to me. Thanks @shred86

[bradjcoughlin]

/update-branch

[mm-cloud-bot]

Test server destroyed

[mm-cloud-bot]

Test server destroyed