talk
talk copied to clipboard
nahid
Add more features to Talk - tag, star, etc
The title says it all, and the file diffs too!
I think this will be an awesome addition to this extremely valuable package.
I erroneously proliferated features in this PR in several places, PR #135 and PR #131.
This PR consolidates the suggestions in comments in those PRs (all now closed in favour of this PR), as well as other code additions and improvements.
@nahid and @cfpinto I apologize for the delay in putting this right. I believe we are all good to merge.
Thanks
@nafiesl @nahid @cfpinto Please I need your opinion: because some of the additions I made add fundamentally different behavior to Talk (e.g. the notifications feature), is it not advisable that merging this PR should necessitate bumping of Talk to the next major version?
hi @damms005 I am not sure what to review here.
the additions I made add fundamentally different behavior to Talk
:point_up_2: are those behaviour changes already discussed with the project maintainer?
Add more features to Talk - tag, star, etc
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.
@nafies He's already aware of some of the changes. I simply moved everything to a single PR so it can be discussed wholistically.
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.
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.
@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)
