-
Notifications
You must be signed in to change notification settings - Fork 9
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
Game Demo Port #611
base: main
Are you sure you want to change the base?
Game Demo Port #611
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #611 +/- ##
==========================================
- Coverage 50.98% 50.26% -0.72%
==========================================
Files 378 391 +13
Lines 32517 33136 +619
==========================================
+ Hits 16578 16657 +79
- Misses 15939 16479 +540 ☔ View full report in Codecov by Sentry. |
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.
Very impressive for a first attempt!
pub fn ui_server_router(port: u16) -> Router { | ||
Router::new() | ||
.route("/index.html", get(index_html)) | ||
.route("/lib/game.min.js", get(game_min_js)) | ||
.route("/lib/game.min.js.map", get(game_min_js_map)) | ||
.route("/lib/swim-core.min.js", get(swim_core_min_js)) | ||
.route("/lib/swim-core.min.js.map", get(swim_core_min_js_map)) | ||
.route("/lib/swim-host.min.js", get(swim_host_min_js)) | ||
.route("/lib/swim-host.min.js.map", get(swim_host_min_js_map)) | ||
.route("/lib/swim-ui.min.js", get(swim_ui_min_js)) | ||
.route("/lib/swim-ui.min.js.map", get(swim_ui_min_js_map)) | ||
.route("/lib/swim-ux.min.js", get(swim_ux_min_js)) | ||
.route("/lib/swim-ux.min.js.map", get(swim_ux_min_js_map)) | ||
.route("/lib/swim-vis.min.js", get(swim_vis_min_js)) | ||
.route("/lib/swim-vis.min.js.map", get(swim_vis_min_js_map)) | ||
.route("/assets/favicon.ico", get(favicon)) | ||
.route("/assets/swim-logo.png", get(swim_logo)) | ||
.with_state(UiConfig { port }) | ||
} |
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 that this is copied from the example app I did but it's definitely hacky. We shoul probably make somethiung more general if we're going to have a lot of these.
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.
Looks really great for a first pass! My comments are just nits really. Before this gets merged It could do with a sprinkle of documentation
// pub total_duration: u64, | ||
// pub last_online: u64, |
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.
Rogue comment
// self.total_duration += match_summary.duration; | ||
// self.last_online = match_summary.end_time; |
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.
Rogue comment
} | ||
|
||
fn configure_logging() -> Result<(), Box<dyn std::error::Error + Send + Sync>> { | ||
let filter = example_filter()? |
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.
You should just be able to depend on the example_util
crate rather than copying this here. You could maybe add a new function there that accepts a slice of additional directives to add
Very rough first draft of the game demo port - needs few iterations of tidying before merging.