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.

Sign up for GitHub

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

Jump to bottom

Replies thread slack like #13805

Closed
wants to merge 11 commits into from
Closed

Replies thread slack like #13805

wants to merge 11 commits into from

Conversation

khaledalyawad
Copy link

@khaledalyawad khaledalyawad commented Mar 20, 2019 •
edited
Loading

Closes #1112

fliptrail and others added 10 commits March 8, 2019 00:31
@fliptrail @ggazzo
[NEW] Bosnian lang (BS) (RocketChat#13635)
8849ece 
This pull request clones Croation (HR) lang to Bosnian lang (BS) to
provide proper UI support for the Bosnian language. For further
information Croation and Bosnian lang are the same, just regional differences between the two exist.
@ggazzo @mrsimpson
@vickyokrm @sampaiodiego
[NEW] Threads (RocketChat#13541)
b93b31b 
Co-authored-by: Oliver Jägle <[email protected]>
Co-authored-by: vickyokrm <[email protected]>
@sampaiodiego
Bump version to 1.0.0-beta.0
69b6b13
@khaledalyawad
replies merge
6e0f5e6
@khaledalyawad
migrating replies
3bcb754
@sampaiodiego @khaledalyawad
Bump version to 1.0.0-beta.1
9d3f6e6
@sampaiodiego @khaledalyawad
Bump version to 1.0.0-beta.2
48082b3
@khaledalyawad
indetation
37f547e
@khaledalyawad
lint
5155c60
@khaledalyawad
lint
6b905a6
@CLAassistant
Copy link

CLAassistant commented Mar 20, 2019 •
edited
Loading

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@khaledalyawad
Copy link
Author

I migrated slack like replies to the latest 1.0.0-beta.2

@khaledalyawad khaledalyawad changed the base branch from develop to beta-release March 20, 2019 05:56
@khaledalyawad khaledalyawad changed the base branch from beta-release to develop March 26, 2019 04:51
@khaledalyawad khaledalyawad changed the base branch from develop to threads March 26, 2019 04:52
@khaledalyawad khaledalyawad mentioned this pull request Mar 26, 2019
Closed
@RocketChat RocketChat deleted a comment Mar 26, 2019
@khaledalyawad khaledalyawad mentioned this pull request Mar 26, 2019
Closed
@mrsimpson
Copy link
Collaborator

Great you give it another shot!
However, you used the code from the old code base, including th assistify-threading package. The content of this packages has been moved to app/threading. Kindly use and modify the files there.

Also, it was help to others of you explained here in some word what you change to achieve what.

@khaledalyawad
Copy link
Author

khaledalyawad commented Mar 26, 2019 •
edited
Loading

it is based of the latest release 63bb92a

mrsimpson
mrsimpson reviewed Mar 26, 2019
View reviewed changes
.meteor/packages
@@ -197,6 +197,7 @@ rocketchat:search
chatpal:search
rocketchat:lazy-load
tap:i18n
assistify:threading
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package should not be there anymore - this is now in /app/discussion.

mrsimpson
mrsimpson reviewed Mar 26, 2019
View reviewed changes
client/methods/addMessageReply.js
@@ -0,0 +1,21 @@
import { Meteor } from 'meteor/meteor';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these files are obsolete and make the changelist longer than it should be

@mrsimpson
Copy link
Collaborator

Hi @khaledalyawad ,
I had a look at the changelist and found there are many files from an older implementation in the packages folder which don't exist anymore.

Thus, I tried to help and merge the current develop, but this caused a lot of other changes. I wanted to see how it works and where the changes are, so I fixed most of them in 95fa16b .

Now, the server technically starts, but I can't get the reply-feature tested. The context menu is now gone.
It may be the result of a wrong merge, so my fault - but you may anyway merge 95fa16b into your branch and continue from there.

@ others: You can see the actual footprint by comparing my commit with develop

@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.

@khaledalyawad
Copy link
Author

@engelgabriel Thanks, I noticed that my name is not there in the release contributors https://github.com/RocketChat/Rocket.Chat/releases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrsimpson mrsimpson mrsimpson left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@khaledalyawad @CLAassistant @mrsimpson @engelgabriel @fliptrail @ggazzo @sampaiodiego