-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
Hey! This is looking great so far - will get a final review in tomorrow and will probably just add some icons in there so that the new items fit in. For members: not sure, will have a think on it - I suppose we could just make it so that clicking the members count or online count opens the members list? |
That's a great idea, I have implemented that in the latest commit! Sounds like the last thing missing here are some icons, wanna add those in? |
Test failure is in the thread_spec, seems like a fluke. Restarting the workflow to see if it fails again! |
Sorry for the slow review here @mxstbr:
|
Working through that list - it's kind of a pain to get the notification controls there for chat, since you'd have to fetch the thread to determine if you are already subscribed or not. That feels like a lot of overhead to add to the page... |
Also the tooltip is hard because that would need to be conditional on both authenticated user and their permissions in the community. E.g. a non-member shouldn't see copy about "channels where you're a member" - for now just going to remove that tooltip entirely |
Pushed a couple small tweaks, but let me know what you think about how to handle the chat notifications @mxstbr :) |
I think we are fine to load the watercooler thread, because then we can also show an "activity dot" if |
@brianlovin latest commits adds the notification action to the chat list item and clean up the UI across the board a bit. Unauthenticated and non-member view: Member view (note the wording change to "All your channels"): I have also added a variable called We should probably also align the channel view to not have any top tabs, link to the members view when clicking on the count and have the same "search" experience on the posts feed. Going to tackle that now! |
Holy crap man! Amazing stuff here, all this looks so good. Let me pull the latest and look into where the new activity dot could go on the chat item. |
@mxstbr d59587e => adds the new activity dot, but intentionally commented it out because we need to perform a separate mutation to make sure that the dot gets cleared out as soon as you view the thread (or at least updates in the background, so that when you click away from chat the dot won't appear there) Does that feel high pri to you, or should we ship this as-is? |
With the channel view modifications @mxstbr it's no longer possible to view a members list of people in a specific channel. This might be frustrating for people who moderate private channels and want to see who has access to that private channel... |
@brianlovin that should still be visible in the channel settings, right? Maybe for admins instead of showing the "Leave" action in the sidebar we show the settings cog for each channel? |
Working on cleaning this up now! 👍 |
Added a settings button to channel list items you are a mod or owner of (at either the community- or channel level): I don't think it's high prio to fix the click-and-notifications-dot-goes-away, let's do that as a follow-up. Will take care of fixing these e2e tests now and then we should be able to ship?! |
@mxstbr I don't think this is the right change - mods still want to leave channels; we had a lot of complaints back in the day about this and that's why we made it so that admins could leave, but we'd throw the modal in front of the leave dialog just in case. I'm not sure the right design here, will think on it today. But in this new design we're sacrificing the ability to manage your channel relationship, get to settings, and view meta info about the channel (members list). |
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.
Let's call this an incremental improvement and go from here.
Status
Deploy after merge (delete what needn't be deployed)
@brianlovin the only thing missing here is the members list: where should we put it?