-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Consistently display script icons for nodes in connect dialog's scene tree editor #92648
Conversation
Looks good! Could you squash the commits? See PR workflow for instructions. |
I will squash them as soon as possible (away from PC til sunday :0) thank you! :) |
79b8342
to
1470247
Compare
After interactive rebasing, I noticed I didn't see my 2 review commits from here. I added them and amended them as a commit to previous. I hope this is okay, and feel free to let me know if there's something else I should have done :) sorry for any inconvenience (and thank you for reviewing!) |
Yes what you did looks correct. We like that simple PRs have a single commit, so fixups following PR review should be amended into the original commit to keep it as a single commit. I pushed another rebase of your branch as there was a temporary CI failure due to GitHub Actions changes, unrelated to this PR. |
Thanks! And congrats for your first merged Godot contribution 🎉 |
#92513 requested to be able to view script icons with/without the ConnectDialog's advanced tab enabled for better context when selecting signals.
The change made was simple, with the biggest change involving the use of 'connecting_signal' boolean to ensure all script icons are displayed (custom or otherwise) when the connect dialog scene tree is open.
Before this change, all icon additions were made under the connect_to_script_mode. When Advanced is enabled, the connect_to_script_mode is set to false, which results in never adding the script buttons to the scene tree editor.
The fix should be low-risk because the icons are only added during connect dialog scene tree's. I would have added tests, but I noticed there are currently no test files under editor/ (and this being my first ticket, I wasn't sure how to proceed with the tests).
The 'before change' video (linked in the original ticket) displays the issue, and below I have the 'after change' result of both advanced off/on.
Advanced off:
Advanced on:
Bugsquad edit: