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

Make "Advanced unit selection" dialog movable #1434

Merged
merged 2 commits into from
Oct 21, 2022

Conversation

Koffy25
Copy link
Contributor

@Koffy25 Koffy25 commented Oct 20, 2022

Closes #1111

@lmoureaux lmoureaux self-requested a review October 20, 2022 20:24
@lmoureaux
Copy link
Contributor

Will test later tonight/tomorrow, for now I noticed that the commit message looks strange, maybe you want to change it?

@Koffy25
Copy link
Contributor Author

Koffy25 commented Oct 20, 2022

Will test later tonight/tomorrow, for now I noticed that the commit message looks strange, maybe you want to change it?

Done.

@Koffy25
Copy link
Contributor Author

Koffy25 commented Oct 21, 2022

I got an email that says the windows clang step failed but it says passed in the pull request so I'll assume all is well.

@lmoureaux
Copy link
Contributor

I got an email that says the windows clang step failed but it says passed in the pull request so I'll assume all is well.

Indeed, I saw it too. Probably an earlier version of the commit, the Windows clang build isn't 100% reliable because of frequent network errors when pulling dependencies.

@jwrober
Copy link
Collaborator

jwrober commented Oct 21, 2022

I don't think its appropriate for me to speak to code quality, however I did test this PR in my local and it does as advertised. The dialog is no longer modal and does allow me to move it around. It may also be a model for us to get rid of the hacked dialogs.

Copy link
Contributor

@lmoureaux lmoureaux left a comment

Choose a reason for hiding this comment

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

Since it was confirmed that this works, my only question is why disband_box was also moved?

@Koffy25
Copy link
Contributor Author

Koffy25 commented Oct 21, 2022

Since it was confirmed that this works, my only question is why disband_box was also moved?

At one time it was getting the "unexpected class name" error but the recursive include issue is fixed now so I'll move it back

@lmoureaux lmoureaux merged commit db1d40a into longturn:master Oct 21, 2022
@lmoureaux
Copy link
Contributor

Thanks!

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.

Make "Advanced unit selection" dialog movable
3 participants