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

Append read receipt feature for #3411 #4047

Closed
wants to merge 2 commits into from

Conversation

dezzzus
Copy link
Contributor

@dezzzus dezzzus commented Aug 17, 2016

@RocketChat/core

Closes #3411

Append read receipt info to message entry.

Append read receipt info to message entry.
@tgxn
Copy link
Contributor

tgxn commented Aug 18, 2016

Hey, this is great, but it looks like it depends on the code in your other PR #4048? (for the setMessageReader function)

@rodrigok
Copy link
Member

Hi @alexdorn87, first of all, thanks by this contribution.

I have some questions/suggestions about this PR

  1. Can you merge your 2 PRs in one since they are dependents?
  2. I like the idea, but this should be optional, can you create a setting for that and disabled by default?
  3. You can improve your code by removing your loop on messages and changing the update to affect all desired messages in one single query, I can help you with this if you want.
  4. Will you change the interface to show this list of users?
  5. Maybe we should add a setting to enable this feature to only channels with a certain number of users, we have bad experiences with large arrays, think in rooms with thousands of users.

@RocketChat/core what do you think?

@geekgonecrazy
Copy link
Contributor

This would be a great addition!

I agree large rooms this could be very bad for experience. Maybe a setting to only enable for rooms with users <= x ? And let users define it. Then default it to like 10-15 ?

@dezzzus
Copy link
Contributor Author

dezzzus commented Aug 18, 2016

hi #rodrigok

Thanks for your detailed message.
1)
Could you help to me to know how to do it, please?

I will make it, but now I am some busy.
If you don't mind, please make it, too.
Or not, I will make it myself
3)
Now I don't know what you mean.
If you can do it, please help me.
thanks
4)
I changed already on my server.
But I don't commit it, because I don't know the design will be accept by your team.
5)
Yes, That's good idea.
I hope to interact with you more and make more completed chat server.
I am appreciate for your open source chat server.
I hope to get more touch with you.

thanks, guys.

@engelgabriel
Copy link
Member

@alexdorn87 can you update all the indentation to TABs?

@xavierzwirtz
Copy link

If I stepped in and cleaned this pull request up would the RocketChat team be willing to pull it in? This feature is vital for the way my team uses chat, but I don't want to be running a custom version.

@engelgabriel engelgabriel added this to the 0.50.0 milestone Jan 16, 2017
@engelgabriel
Copy link
Member

Yes @xavierzwirtz we'd like to add this feature, if you can make the changes, we'd love to merge it.

@xavierzwirtz
Copy link

Awesome.

@alexdorn87, could you post the UI modifications you made somewhere?

@dezzzus
Copy link
Contributor Author

dezzzus commented Jan 18, 2017

Hi @xavierzwirtz

which UI modification do you need?
Could you give me more details, please?

@xavierzwirtz
Copy link

The UI for displaying which users have seen a message. I've got the changes you have pushed cleaned up and rebased on tip. Just need the UI changes.

@engelgabriel engelgabriel modified the milestones: 0.50.0, Short-term Jan 24, 2017
@RichardFoxworthy
Copy link

any progress on this one @alexdorn87 and @xavierzwirtz ? Will be a great new piece of functionality!

@xavierzwirtz
Copy link

I was able to rebase the changes to the server, and I can see the data getting sent back to the server whenever a message is read. However, I am not familiar with the UI framework that is being used within rocket chat, and have not had time to get familiar with it. I was hoping that @alexdorn87 could post up the UI changes that he made here so that I can get them integrated as well.

@dezzzus
Copy link
Contributor Author

dezzzus commented Feb 21, 2017

I posted all files.
if you need more, please tell me more details

@xavierzwirtz
Copy link

@alexdorn87, you said that

I changed (the UI) already on my server.
But I don't commit it, because I don't know the design will be accept by your team.

Could you post the changes that you made for that? I can get them into a PR and have the RocketChat review them.

@RichardFoxworthy
Copy link

Hi again @alexdorn87 and @xavierzwirtz - is there an easy way forward to complete this PR? Would be a great feature addition to Rocket.chat!

@ArchimedesFM
Copy link

This would be a great feature addition to Rocket.chat!

@HammyHavoc
Copy link
Contributor

Would love to see this in RC, and completely agree about keeping it disabled by default, and defaulting to only on rooms with a handful of users.

@engelgabriel
Copy link
Member

@sampaiodiego can you link this on the PR you are about to create for this feature?

@engelgabriel
Copy link
Member

Closed via #9717

@engelgabriel engelgabriel modified the milestones: Short-term, 0.62.0 May 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

read receipts?
9 participants