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

meter shows "finished trips vs baseline" instead of time #565

Merged
merged 2 commits into from
Mar 16, 2021

Conversation

michaelkirk
Copy link
Collaborator

Screen Shot 2021-03-15 at 4 34 47 PM

Screen Shot 2021-03-15 at 4 34 54 PM

trip_meter.mov.mp4

edge cases

next day
Screen Shot 2021-03-15 at 4 25 02 PM
no prebaked day/night
Screen Shot 2021-03-15 at 4 24 24 PM
Screen Shot 2021-03-15 at 4 24 17 PM

no changes
Screen Shot 2021-03-15 at 4 35 52 PM

Copy link
Collaborator

@dabreegster dabreegster left a comment

Choose a reason for hiding this comment

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

Awesome job! I think this strikes a great balance between easy to learn the meaning of (with the tooltip) and succinct to glance at after knowing.

if app.has_prebaked().is_some() {
let now = self.time;
let mut baseline_finished = self.baseline_finished_trips.unwrap_or(0);
for (t, _, _, _) in &app.prebaked().finished_trips[baseline_finished..] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahhh, clever trick. Most of the things that read Analytics recalculate everything from scratch constantly. To one day speed this up, I was imagining making some kind of stateful "cursor" objects to remember where a computation left off. But that's total overkill for something like this -- just remembering the count is much simpler.

}

let text_geom = Text::from(
Line(format!("Finished Trips: {}", prettyprint_usize(finished),)).fg(text_color),
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: stray comma inside the format macro

let mut tooltip_text = Text::from(Line("Finished Trips"));
tooltip_text.add(Line(format!(
"{} ({}% of total)",
finished,
Copy link
Collaborator

Choose a reason for hiding this comment

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

prettyprint_usize(finished)

// TODO: up/down icons
let line = if baseline_finished > finished {
let difference = baseline_finished - finished;
Line(format!("{} less than baseline", difference))
Copy link
Collaborator

Choose a reason for hiding this comment

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

prettyprint_usize here and below, to help point out really dramatically high differences from the baseline

@michaelkirk
Copy link
Collaborator Author

Ok, I believe I've responded to your feedback. PTAL @dabreegster

@dabreegster
Copy link
Collaborator

Thanks for the PR!

@dabreegster dabreegster merged commit 810c89e into a-b-street:master Mar 16, 2021
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.

2 participants