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

Improved Msgbox #464

Closed
wants to merge 9 commits into from
Closed

Improved Msgbox #464

wants to merge 9 commits into from

Conversation

Nico8340
Copy link
Contributor

This PR removes the old msgbox resource and creates a new one that provides a much simpler concept and looks better.
Available types: info, question, warning, error
Available buttons: ok, okcancel, retrycancel, yesno, yesnocancel
Képernyőkép 2024-01-28 012351

@Nico8340
Copy link
Contributor Author

A set of pictures of the new look in action (default resources)
image
image
image

@Fernando-A-Rocha
Copy link
Contributor

Good job! I would just add more spacing between the title and message, as well as the icon on the left @Nico8340

@Nico8340
Copy link
Contributor Author

Good job! I would just add more spacing between the title and message, as well as the icon on the left @Nico8340

Should there be more space vertically or horizontally?
@Fernando-A-Rocha

@Fernando-A-Rocha
Copy link
Contributor

Good job! I would just add more spacing between the title and message, as well as the icon on the left @Nico8340

Should there be more space vertically or horizontally?
@Fernando-A-Rocha

On the y axis between the 2 text labels, and on the x axis between the image and the texts, imo @Nico8340

@Nico8340
Copy link
Contributor Author

Nico8340 commented Jan 28, 2024

On the y axis between the 2 text labels, and on the x axis between the image and the texts, imo @Nico8340

Does they look better now? 😄
@Fernando-A-Rocha
image

@Fernando-A-Rocha
Copy link
Contributor

On the y axis between the 2 text labels, and on the x axis between the image and the texts, imo @Nico8340

Does they look better now? 😄
@Fernando-A-Rocha
image

I think it looks fine now

@Fernando-A-Rocha
Copy link
Contributor

There is no way to handle when a player clicked a specific button like Ok, Cancel? Seems like all they do is close the window

@Nico8340
Copy link
Contributor Author

Nico8340 commented Jan 28, 2024

There is no way to handle when a player clicked a specific button like Ok, Cancel? Seems like all they do is close the window

@Fernando-A-Rocha
Of course, it is possible to handle the buttons, the function returns three variables that contain the buttons, and the onClientGUIClick event can be used to handle the responses. They only closes so that the scripts do not have to handle this separately.

@Nico8340
Copy link
Contributor Author

By the way, I would like to use this system for the new chat implementation as well, in case of mute, notify the player to end this activity or he will face a bigger sanction.

Copy link
Contributor

@jlillis jlillis 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 overall, just a few minor notes. This implementation will work just fine in editor_gui and similar resources, but the admin and admin2 resources have their own unique implementations that make use of coroutines that can't be easily replaced by this resource.

[editor]/editor_gui/client/me_gui.lua Outdated Show resolved Hide resolved
[gameplay]/msgbox/meta.xml Outdated Show resolved Hide resolved
[gameplay]/msgbox/meta.xml Show resolved Hide resolved
[gameplay]/msgbox/sourceC.lua Outdated Show resolved Hide resolved
[gameplay]/msgbox/sourceC.lua Outdated Show resolved Hide resolved
@Nico8340
Copy link
Contributor Author

Nico8340 commented Jan 31, 2024

The changes have been made, but for some reason GitHub says they haven't. (they are now, it was just github's bad ux,.)

@Nico8340 Nico8340 requested a review from jlillis February 1, 2024 01:45
@Nico8340
Copy link
Contributor Author

Nico8340 commented Feb 2, 2024

I'm closing this pull request because I have a much better idea for the implementation that is completely different from this one.

@Nico8340 Nico8340 closed this Feb 2, 2024
@Nico8340 Nico8340 deleted the msgbox branch February 2, 2024 17:56
@Nico8340 Nico8340 mentioned this pull request Feb 4, 2024
4 tasks
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