Skip to content

Conversation

@Ibrahim2750mi
Copy link
Contributor

@Ibrahim2750mi Ibrahim2750mi commented Mar 4, 2023

Please Review
What to review:

  • Check any grammar or spell errors.
  • Check that each diff hunk in menu_0[0-9].py is mentioned in the respective step [0-9].
  • Check any blank lines at the end of the codeblocks
  • Missing punctuation marks and using 'I' instead of 'we'

Also currently waiting for @eruvanos to propose a fix for UIDropdown 🙏🏽 in menu_05.py. The code errors now when operation the UISlider, its because of a bug/wrong code style.

@Ibrahim2750mi Ibrahim2750mi marked this pull request as draft March 4, 2023 21:21
@Ibrahim2750mi Ibrahim2750mi marked this pull request as ready for review March 5, 2023 23:35
@Ibrahim2750mi
Copy link
Contributor Author

The UISlider throwing an exception should be fixed now.

Copy link
Member

@pvcraven pvcraven left a comment

Choose a reason for hiding this comment

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

Is this ready? You've got 1/4 tasks but the PR is not in a 'draft' state, so I'm a bit confused. If it is ready to be included, keep it in the current 'ready' state. Otherwise set to 'draft'.

I'm excited to get a tutorial for GUI in, and this looks good.

@Ibrahim2750mi
Copy link
Contributor Author

Is this ready? You've got 1/4 tasks but the PR is not in a 'draft' state, so I'm a bit confused. If it is ready to be included, keep it in the current 'ready' state. Otherwise set to 'draft'.

It is ready 100%, I have added those checkboxes for the people who choose to review just for double checking my stuff. I will tick those if you say so.

@Ibrahim2750mi
Copy link
Contributor Author

@pvcraven Also don't merge now @eruvanos said he will have a look sometime this week.

@pvcraven
Copy link
Member

pvcraven commented Apr 1, 2023

Looks good to me! I didn't merge because you said "Also don't merge now @eruvanos said he will have a look sometime this week."

@eruvanos
Copy link
Member

eruvanos commented Apr 1, 2023

I have basically the full day tomorrow, I plan to care about all PRs regarding GUI and finish my left over tasks.

@pushfoo
Copy link
Member

pushfoo commented Apr 1, 2023

tl;dr I think this should be simplified and split, but we could do it in subsequent PRs

There are multiple concerns mixed together in this PR which I think should be separate:

  1. A tutorial on how to create a menu screen
  2. Modal dialogs
  3. Maximizing tutorial coverage of our UI widgets

I think we should consider something like the following steps within the framework:

  1. Rethink our widget hierarchy a little, including making a UINinePatchLayout baseclass to encapsulate 9-patch and anchor layout behavior
  2. Add a class UIModal(UINinePatchLayout, UIMouseFilterMixin)
  3. Make UIMessageBox and this PR's SubMenu inherit from UIModal
  4. Create a views index page to aggregate view & menu tutorials like the shader tutorials index page
  5. Split this PR's concerns into a Basic Menu & Advanced Menu tutorial
@eruvanos
Copy link
Member

eruvanos commented Apr 2, 2023

@pushfoo I get your concerns and agree, that we could split up the tutorial even more.
I created a new issue for that: #1667

Still I merge this and we can improve and split up things afterwards, if somebody can spend the time to do so.

@Ibrahim2750mi Thank you for your work!

@eruvanos eruvanos merged commit 0e3de2a into pythonarcade:development Apr 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants