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

feat(natives/ped): add SetPedPhonePaletteIdx #993

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

ahcenezdh
Copy link
Contributor

For js & c# examples i'm not sure at all, someone have to review it to be sure. Close the issue #989

Copy link
Contributor

@4mmonium 4mmonium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there, this replaces a native, therefor the old native file should be removed. Ideally, an alias pointing to the old native name should be added as well since that's standard.

@ahcenezdh
Copy link
Contributor Author

Hey there, this replaces a native, therefor the old native file should be removed. Ideally, an alias pointing to the old native name should be added as well since that's standard.

For x reason when searching it on my visual studio code, couldn't find it

@ahcenezdh
Copy link
Contributor Author

Should be good

@ahcenezdh
Copy link
Contributor Author

I removed the _ from the aliases since this native didn't has a name before

@ahcenezdh
Copy link
Contributor Author

Hey @4mmonium srry to ping you but any news for this PR?

Copy link
Contributor

@4mmonium 4mmonium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is OK, but I'd prefer if you used an enum rather than passing it as a string for hash table (key-value pair tables) lookup. Check my comment, thanks!

PED/SetPedPhonePaletteIdx.md Outdated Show resolved Hide resolved
@ahcenezdh ahcenezdh requested a review from 4mmonium April 21, 2024 13:36
Copy link
Contributor

@4mmonium 4mmonium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, merging, thank you!

@4mmonium 4mmonium merged commit 20ca366 into citizenfx:master Apr 22, 2024
1 check 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