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

Fix menu items that trigger secondary interface missing ellipsis #80355

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Fix menu items that trigger secondary interface missing ellipsis #80355

merged 1 commit into from
Aug 9, 2023

Conversation

jcovin293
Copy link
Contributor

This PR fixes #80347 where menu items that trigger a secondary interface (such as a dialog) do not have an ellipsis on their menu items.

@jcovin293 jcovin293 requested a review from a team as a code owner August 6, 2023 22:29
@dalexeev dalexeev added this to the 4.2 milestone Aug 7, 2023
editor/scene_tree_dock.cpp Outdated Show resolved Hide resolved
@akien-mga akien-mga changed the title Fix Menu Items That Trigger Secondary Interface Missing Ellipsis Fix menu items that trigger secondary interface missing ellipsis Aug 7, 2023
@akien-mga
Copy link
Member

Could you amend the commit message to be more explicit? The PR title is good, the commit should use the same title (and can include "Fixes #80347" in the body of the commit message, not in the title).

@jcovin293
Copy link
Contributor Author

I have amended the commit with the requested changes. Let me know if there are any other things that need to be added.

Copy link
Contributor

@YuriSizov YuriSizov 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 to me, but needs a rebase.

@jcovin293
Copy link
Contributor Author

I'm new to git. How would I rebase because there seems to be a merge conflict in scene_tree_dock.cpp?

@YuriSizov
Copy link
Contributor

That's exactly why you need to do a rebase right now, to resolve those conflicts. When you do a rebase, if there are conflicts that cannot be resolved automatically, you'll be prompted about it and the rebase is going to be put on hold. This allows you to modify the files and decide how to resolve conflicts and then proceed.

What do you use to make the changes and to work with git?

@jcovin293
Copy link
Contributor Author

I use Visual Studio Code. I have tried resolving the conflict via the UI but it doesn't seem to be working as expected as it requires me to make another commit which might not look good on the git history.

@YuriSizov
Copy link
Contributor

YuriSizov commented Aug 9, 2023

@jcovin293 While I'm not sure how it would create a new commit, you can fix that after the fact by doing an interactive rebase and squashing your commits together with the fixup option.

@jcovin293 jcovin293 closed this Aug 9, 2023
@AThousandShips
Copy link
Member

AThousandShips commented Aug 9, 2023

You accidentally closed your PR by resetting the branch, if you push to the branch again we can reopen, alternatively you can open a new PR with a single commit containing the changes

@jcovin293
Copy link
Contributor Author

I have pushed a new commit. Do I need to do something to reopen it or is it on your end?

@AThousandShips AThousandShips reopened this Aug 9, 2023
@YuriSizov
Copy link
Contributor

All is in order now, good job!

@akien-mga akien-mga merged commit 8375f73 into godotengine:master Aug 9, 2023
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@jcovin293 jcovin293 deleted the issue/80347/fix-missing-ellipsis branch August 9, 2023 15:48
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.

Some Menu Items that Trigger a Secondary Interface are Missing an Ellipsis
7 participants