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

Add more features to Talk - tag, star, etc #145

Open
wants to merge 58 commits into
base: master
Choose a base branch
from

Conversation

damms005
Copy link

The title says it all, and the file diffs too!

I think this will be an awesome addition to this extremely valuable package.

@damms005 damms005 force-pushed the master branch 2 times, most recently from f5abc5d to ec4e996 Compare March 29, 2020 14:43
@damms005
Copy link
Author

damms005 commented Mar 29, 2020

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

@nafiesl
Copy link
Contributor

nafiesl commented Mar 30, 2020

hi @damms005 I am not sure what to review here.

the additions I made add fundamentally different behavior to Talk

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

This enables support for the system notification feature, since
(system) notification tags does not necessarily belong to any user.
@damms005
Copy link
Author

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

@damms005
Copy link
Author

damms005 commented Dec 5, 2020

@nafiesl @imanghafoori1 Support now added for up to Laravel v8

@damms005
Copy link
Author

damms005 commented Dec 25, 2022

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

@nahid
Copy link
Owner

nahid commented Dec 25, 2022

I'll let you know after checking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants