Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NEW] Slack Like Threading #13394

Closed
wants to merge 1 commit into from

Conversation

khaledalyawad
Copy link

@khaledalyawad khaledalyawad commented Feb 7, 2019

Closes #1112

threading

@CLAassistant
Copy link

CLAassistant commented Feb 7, 2019

CLA assistant check
All committers have signed the CLA.

@khaledalyawad khaledalyawad changed the base branch from develop to release-0.71.2 February 7, 2019 07:38
@khaledalyawad khaledalyawad changed the base branch from release-0.71.2 to develop February 7, 2019 07:40
@khaledalyawad khaledalyawad mentioned this pull request Feb 7, 2019
@mrsimpson
Copy link
Collaborator

@khaledalyawad thanks for opening the PR - and thanks for cleaning the history so we can see what you did!

Now, I can give you some more profound feedback. I created a screencast (sorry for the slow typing in there), so you can hopefully follow my points. I opened two sessions, user (u) and admin (a) writing some messages
inline-threading

Subscriptions and notifications

You chose a lightweight approach, which means that the thread is not an own entity (such as a room), but merely exists due to the relationship of messages. Compared to making the thread an own room, this is particularly more challenging when it comes to subscribing to it and notifying the user about new messages inside the thread.

Once a new message is sent in a thread by u, a will only be able to notice it once he's got the thread opened. Once he closes the tabbar, there will be a notification in the parent channel, but no indication that the message is inside the thread.

I see a major issue here, since Rocket.Chat's model sees the subscription only at room level. Without a room, there won't be any chance to notify users directly about the thread as an entity.

Ordering

Also, it's a bit tricky when it comes to ordering the messages. Once a thread contains a newer messages compared to the last one in the parent channel, the thread is effectively newer. Thus, it should be ordered below the last message. (at least I'd expect that, MS Teams does this nicely)

However, the ordering is determined by the tsof the message. _updatedAt is not respected here. Changing ts might have unwanted side-effects I cannot foresee.

Scoping

This one is manageable, but tricky in detail: There are various interactions which address the current room, such as the typing indicator (does a user currently respond to the thread or its parent?) Also, I would like to know how you solved the other tabbar's semantics in the thread (how can a user see the attachments/pins/mentions in the thread - and are they hidden from the parent?

Lifecycle

There are a couple of nasty things when changing messages:
What happens on delete of the thread-starting message? Pressing the arrow-up key: which message is the user going to edit? Those are manageable, but need attention as well.

Mobile

Last not least, there are the non-browser clients. How's the display on the apps? I'd expect it to be just as if there was no thread at all: all messages written inside threads and outside in one continuous stream (unless you adapt the apps, of course ;-) ). This makes display not worse than it currently is without threading, but also not better - and finding a good solution for inline threads with limited space is a challenge, at least to me.

Please don't get me wrong: I really think you've done a decent job! Most of the challenges arise from the functionality of lightweight threading itself in combination with Rocket.Chats models.
For these reasons, I opted for heavy threading in #11803.

Nevertheless, I believe one can combine the heavyweight approach with displaying the thread in the tabbar. Having a room which holds the thread's messages, you can have subscriptions and use them to notify and navigate.

Hope I could make me points clear without having you frustrated!

Oliver

@khaledalyawad
Copy link
Author

@mrsimpson thanks for your comment. I understand .... I was thinking to bring the slack reply functionality to rocket.chat

@mrsimpson
Copy link
Collaborator

@khaledalyawad I really didn't want to scare you off! I really think (and this is also feedback from some community members), that the slack-like presentation is welcomed by many!
It's just that the implementation of light threading raises new questions which I wanted to lay down. In the meantime, it seems as if Rocket.Chat was going to really work on implementing threading based on the model suggested in #11803 . If you want to, it should be safe to implement a visualization like the one you went for based on that.
However, if I were you, I'd try to get some feedback from @RocketChat/core before working on that. It feels bad working for the bin stuff that is not going to be merged, at least for me...

@Tirvanel
Copy link

I strongly support this version of threading over the 'Create a new channel for each thread' implementation. This feels a lot more intuitive, and less likely to clutter stuff up. When using slack with friends, we can use 10+ threads in a matter of hours, which would get really unwieldy if it was 10 new channels. With the new channels I think I'd be really likely to get lost on what thread went where.

@geekgonecrazy
Copy link
Member

This type of UI can be implemented potentially with a per channel model implementation.

@pshute
Copy link

pshute commented Mar 17, 2019

"This type of UI can be implemented potentially with a per channel model implementation."

@geekgonecrazy only if it's possible to identify the "parent" channel of any thread channel. Is that possible now that people can rename thread channels?

@sampaiodiego
Copy link
Member

@geekgonecrazy only if it's possible to identify the "parent" channel of any thread channel. Is that possible now that people can rename thread channels?

yes.. thread rooms have a property called prid that represents its parent channel _id

@engelgabriel
Copy link
Member

We have decided that was better to delay our v1 release and get the Threads done properly than get the feature out the way it was initially implemented on the develop branch. We have "rebranded" our current implementation of Threads to Discussions (was it creates a new room type) and will implement Threads exactly the same way as Slack does.

In our vision there are 2 different use cases on how to break out of the main conversation: one very easy and organic, with a single click for small side talk, and other, much more structured, that creates a new conversation with its own life cycle.

We have tried to have a single solution for both, and as a result, no one was happy with it.
So we have given up on having a single approach for both use cases and will implement both: the Slack-like industry standard and our brand new one, called Discussions.

#13843

@engelgabriel engelgabriel added this to the 1.0.0 milestone Mar 21, 2019
@zdumitru
Copy link
Contributor

@khaledalyawad Can you please rebase the PR? It has multiple conflicts.

@geekgonecrazy
Copy link
Member

@ggazzo I know you are working on something like this now. Is this being used as base? Rebasing this likely will be a lot of work so if we can save them the extra time and effort and point to our WIP would be good.

@zdumitru
Copy link
Contributor

zdumitru commented Apr 2, 2019

@khaledalyawad @ggazzo Can you, please, rebase?

@ggazzo ggazzo mentioned this pull request Apr 3, 2019
@engelgabriel
Copy link
Member

Thank you @khaledalyawad for the hard work on this PR and @mrsimpson for the feedback and deep technical considerations about the underlying data model. We have used this code as the bases for our implementation and the feedback to make improvements to address most of the listed issues and concerns.

The result is the ongoing PR #13996 bu @ggazzo which will be marked as coauthored with you both.

Thank you again for helping us build such a complex and highly demanded feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Threads
10 participants