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

Root state and simple menu with credits #16

Merged
merged 20 commits into from
Jul 5, 2024

Conversation

MiniaczQ
Copy link
Collaborator

@MiniaczQ MiniaczQ commented Jul 4, 2024

Fixes #3, #9
Partially: #4

Based on https://github.com/MiniaczQ/bevy-substate-based-ui

This is very experimental, feel free to propose large changes to the structure

@janhohenheim
Copy link
Member

I'm fine with merging it and then iterating on that. Code looks tidy :)

src/core/menu/credits.rs Outdated Show resolved Hide resolved
src/core/menu/credits.rs Outdated Show resolved Hide resolved
src/core/game/mod.rs Outdated Show resolved Hide resolved
src/core/menu/credits.rs Outdated Show resolved Hide resolved
src/core/menu/mod.rs Outdated Show resolved Hide resolved
src/core/mod.rs Outdated Show resolved Hide resolved
src/ui/mouse_hover.rs Outdated Show resolved Hide resolved
@MiniaczQ
Copy link
Collaborator Author

MiniaczQ commented Jul 5, 2024

More changes than I originally planned, requesting re-review

Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

Great! Got some nitpicks, but I'd prefer to iterate on that once it's merged so that we have a basic foundation :)

src/ui/mouse_hover.rs Outdated Show resolved Hide resolved
src/ui/mouse_hover.rs Outdated Show resolved Hide resolved
src/ui/widgets.rs Show resolved Hide resolved
src/ui/mod.rs Show resolved Hide resolved

use bevy::{color::palettes::tailwind, prelude::*};

pub use widgets::*;
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO it would be better to pub mod widgets instead. But that's just my opinion, and can be discussed after this PR is merged.

src/core/mod.rs Outdated Show resolved Hide resolved
src/core/menu/mod.rs Outdated Show resolved Hide resolved
Credits,
}

fn setup(mut commands: Commands) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-specific system names like "set up" and "update" are confusing, and encourage putting everything in one system instead of splitting into multiple systems. Fine to keep it this way for this PR, though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, but overly specific names without context would be weird too. Adding some actual game logic would show both sides

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a comment for game logic, hope we can iterate on this further though

src/core/menu/credits.rs Outdated Show resolved Hide resolved
src/core/menu/credits.rs Outdated Show resolved Hide resolved
@janhohenheim
Copy link
Member

@benfrankel merging this since I think your concerns have been addressed and you mentioned that you are fine with resolving some nitpicks with follow-up PRs. Hope you're alright with that :)

@MiniaczQ MiniaczQ merged commit 8ec2a68 into TheBevyFlock:main Jul 5, 2024
3 checks passed
@MiniaczQ MiniaczQ deleted the simple-ui branch July 5, 2024 12:47
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.

Add credits menu
3 participants