-
-
Notifications
You must be signed in to change notification settings - Fork 438
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
onPlayerTeamChange Event #3370
onPlayerTeamChange Event #3370
Conversation
How about enabling cancelEvent() for this new event? |
Hello @esmail9900 and thank you for your PR. Your PR gave me some strong flashbacks from when I attempted to add this event and lost motivation. Please view the comments on this PR #10 and check if your code complies with all that have been said in this PR. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some stuff needs to be adressed.
I think the this event do not need to be cancelable right away, this can be added later with another PR as you will surely encounter some tricky problems to overcome to make it work properly.
Note: What happens if you delete the team ?
(Expectected behaviour would be that this event gets called for every players in the team and before onElementDestroy gets called so that it is already at the right place if you or someone else decides to make your event cancelable)
Added cancelEvent() Before team getting destroyed, event will trigger for all players in the team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks pretty good. Added some more comments.
but this need to be tested when I have time (or someone else ? be my guest)
oldTeam changed to team element instead of nil when team getting destroyed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested it locally and it works fine. Cancelling the event works too and the event is called when the team is deleted as well.
I think this PR will be ready for merge once you resolve the latest things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot some of my suggestions.
If they have been left on purpose, please explain why.
Co-authored-by: Citizen01 <[email protected]>
|
mb, i just confused, im new to github :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay for me. Congrats on your 1st PR.
I'll ask for another review just in case. It can take a bit of time so don't worry if nothing happens in the next few days.
Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Moving CLuaArguments creation outside the loop and calling 'DeleteArguments()' within each iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 🎉
There are enough passed code reviews, thanks for your contribution @esmail9900 |
implemented the onPlayerTeamChange event for the server-side. this event includes two arguments: oldTeam and newTeam. if the player doesn't have an old team, oldTeam will be false.