mattermost-webapp
mattermost-webapp copied to clipboard
MM: 42267: Browse channel UI Update and ability to hide joined channels
Summary
-
Update the modal to the new modal infrastructure to take advantage of the latest modal styling updates.
-
Within the modal, add channel type icon and member count alongside the channel purpose for each channel:
-
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.
-
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:
-
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
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: 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
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.
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.
/update-branch
Nice work @sinansonmez, few things I noticed on a brief test:
-
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:
-
If the channel has no purpose set, we can probably hide the dot here:
-
The number of channels doesn't seem to be displaying:
-
I think the UI might need a bit of clean-up to match the designs, but I'll defer to @matthewbirtch for details.
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 Please ping me once you are done
~~@esethna @matthewbirtch I need to put the image below to when there is no channel. Where can I find the image?~~
Edit: I was able to export it from Figma. Then after converting to React component, I was able to use it
This is ready for the first round of review @esethna @AshishDhama @koox00 @matthewbirtch
Thanks for the detailed review @matthewbirtch. I will let you know when I address all of your points.
@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.
- 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.
- 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.
- 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.
@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:
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 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
@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
/update-branch
@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
Test server creation failed. See the logs for more information.
Creating a new SpinWick test server using Mattermost Cloud.
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 |
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
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.
@sinansonmez great work on this! I just noticed the join button looks a little funky, but looks great otherwise!
I'll leave the remaining reviews to @EricssonLiu @AshishDhama @jgilliam17
@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?
thanks @esethna for the catch. I fixed it with my last commit
@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 opened a ticket about adding infinite scroll and tagged it as HW.
@AshishDhama @EricssonLiu kindly pinging for review
New commit detected. SpinWick will upgrade if the updated docker image is available.
Mattermost test server updated with git commit be7cc7eae51b93826ed0fee6b9694aaba5f210c6
.
Access here: https://mattermost-webapp-pr-11262.test.mattermost.cloud
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!
Sorry @matthewbirtch by mistake I re-requested your review
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
New commit detected. SpinWick will upgrade if the updated docker image is available.
Mattermost test server updated with git commit 33bbf784d3dc71a8677efd04d9e0acfc3327809f
.
Access here: https://mattermost-webapp-pr-11262.test.mattermost.cloud