-
Notifications
You must be signed in to change notification settings - Fork 366
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
Escape menu window #2590
Escape menu window #2590
Conversation
Fixes issue #2546 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the original EscapeMenu
class be deleted in this PR too?
_charselect.IsDisabled = true; | ||
} | ||
|
||
_versionPanel = new VersionPanel(canvas, name: nameof(_versionPanel)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the version panel showing up underneath the modal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into this. The only escapemenu had it float at the very bottom of the screen but the window isn't large enough with this version. I'll likely create an extra imagepanel and add it as a child of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version panel needs to be a child of the modal in this case, rather than another ImagePanel
.
private readonly Button _settings; | ||
private readonly Button _charselect; | ||
private readonly Button _logout; | ||
private readonly Button _exitdesktop; | ||
private readonly Button _closemenu; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make the names be camel-case and more thorough? e.g. _returnToCharacterSelectButton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are tied to the UI files.
I can, but then I have to hardcode the string values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize they're tied to the UI files, but I'm ok with invalidating existing stuff (especially since the entire UI system needs to be replaced anyway).
|
||
public EscapeMenuWindow(Canvas canvas, Func<SettingsWindow> settingsWindowProvider) | ||
: base(canvas, Strings.EscapeMenu.Title, false, | ||
$"{nameof(EscapeMenuWindow)}_{(ClientContext.IsSinglePlayer ? "singleplayer" : "online")}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I didn't even think about this having two separate layouts
With accompanied Designer file Signed-off-by: Derek Gooding <djgooding@live.com>
b5db27f
to
0b01984
Compare
3rd times the charm.
Create a new Escape Menu window using the designer pattern. Call it instead of the old version.