Skip to content

Tutorial for Menu #1608

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

Merged
merged 24 commits into from
Apr 2, 2023
Merged

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants