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.

Sign up for GitHub

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

Jump to bottom

shared: remove is prefix from JavaScript class properties #192

Closed
wants to merge 9 commits into from
Closed

shared: remove is prefix from JavaScript class properties #192

wants to merge 9 commits into from

Conversation

carlos-menezes
Copy link
Contributor

Closes #182

@carlos-menezes carlos-menezes marked this pull request as ready for review February 25, 2024 00:34
xLuxy
xLuxy reviewed Feb 25, 2024
View reviewed changes
Copy link
Collaborator

@xLuxy xLuxy left a comment

Choose a reason for hiding this comment

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

Please go through all v1 bindings (source code, not typings) and fix compatiblity classes. For example, with your change, you're breaking alt.isClient / alt.isServer - and there are more.

@carlos-menezes carlos-menezes marked this pull request as draft February 25, 2024 21:27
@carlos-menezes carlos-menezes marked this pull request as draft February 25, 2024 21:27
@Yiin
Copy link
Contributor

Yiin commented Mar 2, 2024

@carlos-menezes remove "Draft" tag once it's ready for review 👍

@carlos-menezes
Copy link
Contributor Author

Sorry for the delay, @Yiin . Will look into the rest of this PR this weekend.

@xLuxy
Copy link
Collaborator

xLuxy commented Mar 19, 2024

Hey there, any progress on this?

@carlos-menezes
Copy link
Contributor Author

carlos-menezes commented Mar 19, 2024 •
edited
Loading

Really sorry for the delay, these last few weeks have been crazy. Anyway, I'm marking this a ready for review after the last commit. Let me know about any wrong-doings.

edit: updates have been processing for a while now.

xLuxy
xLuxy requested changes Mar 19, 2024
View reviewed changes
Copy link
Collaborator

@xLuxy xLuxy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We should keep methods prefixed with is since they're more fitting in most cases.

Also, please make sure typings are updated accordingly aswell as all binding files are updated to reflect changes - your changes potentially break things there.

Just to name a few breaking things:
img

client/js/compatibility/classes/webview.js Outdated Show resolved Hide resolved
client/src/classes/RmlElement.cpp Outdated
@@ -103,7 +103,7 @@ extern js::Class rmlElementClass("RmlElement", &baseObjectClass, nullptr, [](js:
tpl.Method<&alt::IRmlElement::RemovePseudoClass>("removePseudoClass");
tpl.Method<&alt::IRmlElement::HasPseudoClass>("hasPseudoClass");
tpl.Method<&alt::IRmlElement::SetOffset>("setOffset");
tpl.Method<&alt::IRmlElement::IsPointWithinElement>("isPointWithinElement");
tpl.Method<&alt::IRmlElement::IsPointWithinElement>("pointWithinElement");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe should still be prefixed with is here because it's a method and is more fitting?

server/src/classes/VoiceChannel.cpp Outdated
@@ -19,7 +19,7 @@ extern js::Class voiceChannelClass("VoiceChannel", &baseObjectClass, nullptr, []
tpl.Method<&alt::IVoiceChannel::HasPlayer>("hasPlayer");
tpl.Method<&alt::IVoiceChannel::AddPlayer>("addPlayer");
tpl.Method<&alt::IVoiceChannel::RemovePlayer>("removePlayer");
tpl.Method<&alt::IVoiceChannel::IsPlayerMuted>("isPlayerMuted");
tpl.Method<&alt::IVoiceChannel::IsPlayerMuted>("playerMuted");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe should still be prefixed with is here because it's a method and is more fitting?

@carlos-menezes carlos-menezes mentioned this pull request Mar 19, 2024
Merged
@xLuxy xLuxy mentioned this pull request Jun 15, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xLuxy xLuxy xLuxy requested changes

@xxshady xxshady Awaiting requested review from xxshady xxshady will be requested when the pull request is marked ready for review xxshady is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove is prefix from boolean properties
3 participants
@carlos-menezes @Yiin @xLuxy