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

Escape menu window #2590

Merged
merged 9 commits into from
Feb 27, 2025
Merged

Conversation

DerekGooding
Copy link
Contributor

3rd times the charm.

Create a new Escape Menu window using the designer pattern. Call it instead of the old version.

@DerekGooding
Copy link
Contributor Author

Fixes issue #2546

Copy link
Member

@lodicolo lodicolo left a 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));
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Comment on lines 14 to 18
private readonly Button _settings;
private readonly Button _charselect;
private readonly Button _logout;
private readonly Button _exitdesktop;
private readonly Button _closemenu;
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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")}")
Copy link
Member

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

lodicolo added a commit to AscensionGameDev/Intersect-Assets that referenced this pull request Feb 27, 2025
@lodicolo lodicolo merged commit 4fbff7c into AscensionGameDev:main Feb 27, 2025
1 check passed
@lodicolo lodicolo linked an issue Feb 27, 2025 that may be closed by this pull request
1 task
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.

bug: Escape menu bad alignment
2 participants