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

ci(seatbelt): pass seatbeltOn boolean as a param when calling the seatbelt:client:ToggleSeatbelt event #478

Merged
merged 2 commits into from
Nov 8, 2024

Conversation

vipexv
Copy link
Contributor

@vipexv vipexv commented Nov 1, 2024

Why are we not passing the seatbeltOn bool as a param when calling the seatbelt:client:ToggleSeatbelt event, like what is the benefit in not doing that?

This change simply passes it as a param, and it doesn't break anything, it simply adds an extra feature, and it is an optional param that players/developers can use to make their life easier.

Questions (please complete the following information):

  • Have you personally loaded this code into an updated qbcore project and checked all it's functionality? Yes
  • Does your code fit the style guidelines? Yes
  • Does your PR fit the contribution guidelines? Yes

@Qwerty1Verified
Copy link

I don't mind the idea, could be helpful for clear declaration of data being derived from one source. I do want to know which use case you see for this to be a functional improvement for since you mentioned it's an extra feature?

@vipexv
Copy link
Contributor Author

vipexv commented Nov 8, 2024

I was working on a commission recently, and i found it to be easier to just handle it this way, it just generally doesn't have any cons to it, it only brings benefits and makes things easier.

@Qwerty1Verified
Copy link

Qwerty1Verified commented Nov 8, 2024

Yeah that's what I'm referring to I'd just like to hear your use case on it to know how you used it and how it helped you. Was it to update states on a UI without storing them yourself?

Most other use cases would pretty much require you to store a variable yourself which doesn't change too much.

@vipexv
Copy link
Contributor Author

vipexv commented Nov 8, 2024

For me personally, especially when debugging and having to restart the resource multiple times, it caused the seatbelt to be offsync, there's other use cases for this like you stated, if someone wants to instantly update their Interface without storing a variable they can do that, but i would rather have it guaranteed to be synced rather than just toggling it and your never really 100% sure if its synced, and yes 99.99% of the time it will be, but i would rather have that 100% mark :o

And looking at it, there is no drawbacks to this, and it causes no compatibility issues.

@vipexv
Copy link
Contributor Author

vipexv commented Nov 8, 2024

Even when your storing a variable, i personally would it rather pass it as a prop/argument that is always synced.

@Qwerty1Verified
Copy link

Sounds good, just wanted to make sure it had an actual use case. Although it could be said that state desync isn't a big issue since resources shouldn't really be restarted in "production", I do often argue that if need be then sometimes it must happen for the show to roll on. Then there's the other use case I can think of where you're just handing it off to a UI and don't want to worry about storing it, and updating it.

@vipexv
Copy link
Contributor Author

vipexv commented Nov 8, 2024

For me it's more convenient this way, and it doesn't cost or break anything, which is mostly the primary reason for the suggestion, it simply adds something extra which doesn't break compatibility at all, and can optionally be used.

@Qwerty1Verified
Copy link

Qwerty1Verified commented Nov 8, 2024

Yes, it's more of a development quality of life change, with better practice of handling data at one source.

@GhzGarage GhzGarage merged commit 00e9235 into qbcore-framework:main Nov 8, 2024
1 of 2 checks passed
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