Add more features to Talk - tag, star, etc#145
Add more features to Talk - tag, star, etc#145damms005 wants to merge 58 commits intonahid:masterfrom
Conversation
f5abc5d to
ec4e996
Compare
|
hi @damms005 I am not sure what to review here.
👆 are those behaviour changes already discussed with the project maintainer?
Regarding the PR title, I am suggesting you create different PR of each feature you contribute. So the project maintainer can easily track the changes as it needed. |
This enables support for the system notification feature, since (system) notification tags does not necessarily belong to any user.
|
@nafies He's already aware of some of the changes. I simply moved everything to a single PR so it can be discussed wholistically.
I think it is wrong for me to call it "features" in this PR title, because it is just a single "feature": the Tag feature. This is the main feature in this PR. So while working on this tag feature, I did some refactoring to make it easy to work with Talk. I then built on this new Tag feature to give an example of how it can be used (hence the "Star"-ing of conversation was included, system notification example, etc). All the changes I made are all about the same single feature: Tag. The other one is being able to add "Title" to a conversation, which was already discussed in PR#131. |
|
@nafiesl @imanghafoori1 Support now added for up to Laravel v8 |
|
@nahid @nafiesl It's been a while this issue is opened. I hope I can convince you to merge it; else we should rather close it. I want us to agree on a tradeoff and have a midpoint to resolve this. It was my mistake and big oversight to not have made my changes modular and opened smaller PRs, rather than this large one. I sincerely do not have the bandwidth and resources to rework this to correct this mistake. However, Talk already has extensive and significant test coverage. If I add tests that cover the features in this PR and update the README as necessary, will this be enough to get this PR merged? (this way we can make it a major release rather than a minor update, and I will make time to maintain it in the initial period of such major release) |
|
I'll let you know after checking |
The title says it all, and the file diffs too!
I think this will be an awesome addition to this extremely valuable package.