mattermost-webapp icon indicating copy to clipboard operation
mattermost-webapp copied to clipboard

MM: 42267: Browse channel UI Update and ability to hide joined channels

Open sinansonmez opened this issue 2 years ago • 23 comments

Summary

  1. Update the modal to the new modal infrastructure to take advantage of the latest modal styling updates.

  2. Within the modal, add channel type icon and member count alongside the channel purpose for each channel:

  3. For channels that a user is already a member of, add a visual indicator of this instead of hiding the channel from the list. There should be no “Join” button and instead there should be a hover state button to view the channel.

    • When a user joins a channel via the modal, they should remain in the modal instead of being sent to the channel they just joined.
  4. For channels that have been archived, add a visual indicator of this instead of hiding the channel from the list. The hover state again should be to view the channel, since they cannot join:

  5. Add Checkbox for “Hide joined”. (this item is transferred from 2nd part of this issue of issue to this PR)

Ticket Link

Fixes https://github.com/mattermost/mattermost-server/issues/21173 JIRA: https://mattermost.atlassian.net/browse/MM-42267

Screenshots

before Screen Shot 2022-10-03 at 22 38 56

after

https://user-images.githubusercontent.com/37421564/195212534-500c383c-f063-4bec-89da-f224184b95db.mov

Release Note

Browser channel user interface is updated. Joined channels are displayed. Option to hide joined channels are added.

sinansonmez avatar Oct 03 '22 20:10 sinansonmez

@sinansonmez: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

mm-cloud-bot avatar Oct 03 '22 20:10 mm-cloud-bot

Hello @sinansonmez,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

mattermod avatar Oct 03 '22 20:10 mattermod

E2E tests not automatically triggered, because PR has no approval yet. Please ask a developer to review and then try again to attach the QA label.

mattermod avatar Oct 03 '22 20:10 mattermod

/update-branch

sinansonmez avatar Oct 03 '22 21:10 sinansonmez

Nice work @sinansonmez, few things I noticed on a brief test:

  1. When typing a channel name that matches one a user has already joined, we should show it as joined rather than not show it at all: image

  2. If the channel has no purpose set, we can probably hide the dot here: image

  3. The number of channels doesn't seem to be displaying: image

  4. I think the UI might need a bit of clean-up to match the designs, but I'll defer to @matthewbirtch for details.

esethna avatar Oct 03 '22 21:10 esethna

Thanks @esethna 1, 2, and 3 are noted. For 4th one, I will ask @matthewbirtch to review, when I am done with the PR

sinansonmez avatar Oct 03 '22 21:10 sinansonmez

@sinansonmez Please ping me once you are done

AshishDhama avatar Oct 04 '22 18:10 AshishDhama

~~@esethna @matthewbirtch I need to put the image below to when there is no channel. Where can I find the image?~~ Screen Shot 2022-10-09 at 22 59 55

Edit: I was able to export it from Figma. Then after converting to React component, I was able to use it

sinansonmez avatar Oct 09 '22 21:10 sinansonmez

This is ready for the first round of review @esethna @AshishDhama @koox00 @matthewbirtch

sinansonmez avatar Oct 11 '22 22:10 sinansonmez

Thanks for the detailed review @matthewbirtch. I will let you know when I address all of your points.

sinansonmez avatar Oct 15 '22 13:10 sinansonmez

@matthewbirtch All of your points are addressed (except 5, please see my comment below). Please let me know if there is still something that is still missing.

  1. Designs have the Sort: dropdown. Was that intentionally left out of this PR?

Yes, this PR was separated into 2 parts. This is the first part.

  1. i)Sort functionality, ii) adding Private Channels and All Channel Types to Show: Public Channels menu and iii) Hide Joined checkbox is part of 2nd part. I just moved Hide Joined checkbox implementation to 1st part.
  1. There seems to be some sort of footer element at the bottom of the modal (called filter-controls that is blocking scrollable content at the bottom. Is this needed?

Yes, this div is including Next and Previous buttons (please see in the screenshot below, next is hover state). However, I reduced it size to minimal, so it is not that visible anymore. Next and Previous buttons were not included in the design. Please let me know if you have any feedback about them. Screen Shot 2022-10-16 at 15 21 12

sinansonmez avatar Oct 16 '22 21:10 sinansonmez

@matthewbirtch @sinansonmez should we allow the entire row to be clickable to open an already joined channel?


Separately, I think there might also be a max modal height issue: image


Yes, this div is including Next and Previous buttons (please see in the screenshot below, next is hover state). However, I reduced it size to minimal, so it is not that visible anymore. Next and Previous buttons were not included in the design. Please let me know if you have any feedback about them.

Would it be a big lift to bring us to infinite scrolling in this list? This could be a separate ticket if so. For an example reference, we offer infinite scrolling in the Threads list with CRT enabled. I'm not sure if any other dialogs in the app use infinite scrolling yet, but we should definitely move in that direction

esethna avatar Oct 24 '22 20:10 esethna

@esethna Thanks for the feedback

should we allow the entire row to be clickable to open an already joined channel?

From my perspective, it makes sense to provide a consistent behaviour whether joined or not

Separately, I think there might also be a max modal height issue:

I thought I fixed the max-height issue. I will check again.

Would it be a big lift to bring us to infinite scrolling in this list?

It might be a big lift which might require some changes in GenericModal so I suggest creating a separate ticket for that

sinansonmez avatar Oct 24 '22 20:10 sinansonmez

@matthewbirtch @sinansonmez should we allow the entire row to be clickable to open an already joined channel?

I would kind of expect the following:

  • if you're joined already, the whole row takes you to the channel
  • if you're not joined, the whole row previews the channel

matthewbirtch avatar Oct 26 '22 19:10 matthewbirtch

/update-branch

sinansonmez avatar Oct 26 '22 20:10 sinansonmez

@esethna max-height issue should be fixed now, can you check again 🤞

@matthewbirtch related to your comments

if you're joined already, the whole row takes you to the channel

With latest changes, this is the current behavior

if you're not joined, the whole row previews the channel

Current behavior is the same as clicking Join button. Previewing channel should be implemented in the scope of another ticket

sinansonmez avatar Oct 26 '22 22:10 sinansonmez

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

mm-cloud-bot avatar Oct 26 '22 22:10 mm-cloud-bot

Creating a new SpinWick test server using Mattermost Cloud.

mm-cloud-bot avatar Oct 26 '22 22:10 mm-cloud-bot

Mattermost test server created! :tada:

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

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

mm-cloud-bot avatar Oct 26 '22 22:10 mm-cloud-bot

Thanks @matthewbirtch, I appreciate your comments directly on CSS. All of your points are addressed.

I had a bit of issue with fixing your 1st comment below, especially on the bigger screens

I noticed a new issue with the scrollview content displaying outside of the scrollable area

It seems like two height setting shown below were clashing. It seems like I fixed now, but I don't feel this was the best possible way to fix it. Maybe you or devs can check it to improve it if possible.

https://github.com/mattermost/mattermost-webapp/blob/24278ea29f0779edcb82f5fe7ade0e4093565c4f/components/more_channels/more_channels.scss#L6

https://github.com/mattermost/mattermost-webapp/blob/24278ea29f0779edcb82f5fe7ade0e4093565c4f/components/more_channels/more_channels.scss#L124

sinansonmez avatar Oct 30 '22 14:10 sinansonmez

I had a bit of issue with fixing your 1st comment below, especially on the bigger screens

I noticed a new issue with the scrollview content displaying outside of the scrollable area

It seems like two height setting shown below were clashing. It seems like I fixed now, but I don't feel this was the best possible way to fix it. Maybe you or devs can check it to improve it if possible.

I wasn't entirely sure how to fix this one either. Maybe @AshishDhama can review this and provide some help here as I'm not super familiar with how this height is calculated or how best to resolve.

matthewbirtch avatar Oct 30 '22 15:10 matthewbirtch

@sinansonmez great work on this! I just noticed the join button looks a little funky, but looks great otherwise! image

esethna avatar Oct 31 '22 22:10 esethna

I'll leave the remaining reviews to @EricssonLiu @AshishDhama @jgilliam17

esethna avatar Oct 31 '22 22:10 esethna

@sinansonmez great work on this! I just noticed the join button looks a little funky, but looks great otherwise!

Great catch @esethna. I missed that in the latest update.

Now that I see pagination here, it feels pretty awkward (although I know it's not that different from what we currently have. I agree with Eric that infinite scroll would be way better here. @esethna did you create a follow-up ticket for that?

matthewbirtch avatar Nov 01 '22 13:11 matthewbirtch

thanks @esethna for the catch. I fixed it with my last commit

sinansonmez avatar Nov 01 '22 19:11 sinansonmez

@esethna did you create a follow-up ticket for that?

No I haven't. @Rishselva can you help open a ticket for adding infinite scroll to this modal so we can open as HW? Here's an example: https://mattermost.atlassian.net/browse/MM-4687

esethna avatar Nov 01 '22 21:11 esethna

@esethna opened a ticket about adding infinite scroll and tagged it as HW.

Rishselva avatar Nov 02 '22 13:11 Rishselva

@AshishDhama @EricssonLiu kindly pinging for review

sinansonmez avatar Nov 10 '22 21:11 sinansonmez

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

mm-cloud-bot avatar Nov 10 '22 21:11 mm-cloud-bot

Mattermost test server updated with git commit be7cc7eae51b93826ed0fee6b9694aaba5f210c6.

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

mm-cloud-bot avatar Nov 10 '22 21:11 mm-cloud-bot

Adding @Rishselva to help usher this in. @sinansonmez feel free to reach out to her if this PR is getting blocked. Appreciate your help here!

esethna avatar Nov 10 '22 22:11 esethna

Sorry @matthewbirtch by mistake I re-requested your review

sinansonmez avatar Nov 15 '22 21:11 sinansonmez

I had a bit of issue with fixing your 1st comment below, especially on the bigger screens

I noticed a new issue with the scrollview content displaying outside of the scrollable area

It seems like two height setting shown below were clashing. It seems like I fixed now, but I don't feel this was the best possible way to fix it. Maybe you or devs can check it to improve it if possible.

I wasn't entirely sure how to fix this one either. Maybe @AshishDhama can review this and provide some help here as I'm not super familiar with how this height is calculated or how best to resolve.

@AshishDhama Any help to fix this issue is much appreciated

sinansonmez avatar Nov 21 '22 20:11 sinansonmez

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

mm-cloud-bot avatar Nov 21 '22 21:11 mm-cloud-bot

Mattermost test server updated with git commit 33bbf784d3dc71a8677efd04d9e0acfc3327809f.

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

mm-cloud-bot avatar Nov 21 '22 21:11 mm-cloud-bot