-
Notifications
You must be signed in to change notification settings - Fork 326
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
Add more features to Talk - tag, star, etc #145
base: master
Are you sure you want to change the base?
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.