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

Relay useful PopupMenu signals through MenuButton #90772

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ckaiser
Copy link
Contributor

@ckaiser ckaiser commented Apr 16, 2024

Really simple relaying of the PopupMenu signals through MenuButton, just to make hooking them up through the editor simpler, I know that you can easily connect them through code by using get_popup() but the fact that the menu button doesn't follow the same convention as most of the UI elements where you can just connect through the editor seems to be a source of confusion for new users (see example 1, 2, 3), these from just a few seconds of googling.

The one thing I'm not sure about is the documentation, I didn't want to copy-paste the PopupMenu docs so I just linked to the appropriate signal, but I'm not 100% on the wording or if that's advised.

Let me know if this needs a proposal, figured it was a small enough QOL change that it wouldn't necessarily warrant one.

@ckaiser ckaiser requested review from a team as code owners April 16, 2024 21:08
@KoBeWi KoBeWi added this to the 4.x milestone Apr 16, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Apr 16, 2024

Let me know if this needs a proposal, figured it was a small enough QOL change that it wouldn't necessarily warrant one.

If users are confused on how to use a node, the first step should be improving the documentation. The description of MenuButton could explain how to use popup signals.

Also, while your new signals forward the main popup's signals (which btw isn't something we do in other classes), it won't handle sub-menus, so this change alone doesn't help fully utilizing this node.

@ckaiser
Copy link
Contributor Author

ckaiser commented Apr 17, 2024

Documentation would definitely help, my main gripe with how it's handled right now is consistency.

The case I ran into was in making a menu bar with a few buttons, I could connect them all via the editor with no code except for the MenuButton.

For submenus you already have to dip into GDScript to create and insert it into the popup, so it'd be natural to have the signals connected in code.

Otherwise I'd, for example, have a script where only one of the signals is connected with code and the rest with the editor. It's not really a big deal, but I feel like it's better for simplicity and consistency with other control nodes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants