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

Progress bar #519

Merged
merged 9 commits into from
Jul 1, 2021
Merged

Progress bar #519

merged 9 commits into from
Jul 1, 2021

Conversation

EmbersArc
Copy link
Contributor

Closes #496.

simplescreenrecorder-2021-06-26_22.56.43.mp4

@EmbersArc EmbersArc mentioned this pull request Jun 26, 2021
@parasyte
Copy link
Contributor

The animation looks awesome! I think I like it a little better with the selection background and text colors. What do you think?

egui-progress-dark

egui-progress-light

The diff is:

diff --git a/egui_demo_lib/src/apps/demo/progress_bar.rs b/egui_demo_lib/src/apps/demo/progress_bar.rs
index f0cb976..f37120f 100644
--- a/egui_demo_lib/src/apps/demo/progress_bar.rs
+++ b/egui_demo_lib/src/apps/demo/progress_bar.rs
@@ -72,11 +72,7 @@ impl Widget for ProgressBar {
             ),
         );
 
-        let (dark, bright) = if visuals.dark_mode {
-            (0.2, 0.3)
-        } else {
-            (0.8, 0.9)
-        };
+        let (dark, bright) = (0.8, 1.0);
         let color_factor = if animate {
             ui.ctx().request_repaint();
             lerp(dark..=bright, ui.input().time.cos().abs())
@@ -87,7 +83,7 @@ impl Widget for ProgressBar {
         ui.painter().rect(
             inner_rect,
             corner_radius,
-            Color32::from_gray((color_factor * 255.0) as u8),
+            Color32::from(Rgba::from(visuals.selection.bg_fill) * color_factor as f32),
             Stroke::none(),
         );
 
@@ -119,7 +115,9 @@ impl Widget for ProgressBar {
                 Align2::LEFT_CENTER,
                 text,
                 TextStyle::Button,
-                visuals.text_color(),
+                visuals
+                    .override_text_color
+                    .unwrap_or(visuals.selection.stroke.color),
             );
         }
 

Also, shouldn't the widget be included in egui::widgets and not just the demo lib?

@EmbersArc
Copy link
Contributor Author

@parasyte Much better! I applied the suggestions.

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

This looks really good!

egui/src/widgets/progress_bar.rs Show resolved Hide resolved
egui/src/widgets/progress_bar.rs Outdated Show resolved Hide resolved
}

/// Whether to display a loading animation. Note that this require the UI to be redrawn.
/// Defaults to `false`.
Copy link
Owner

Choose a reason for hiding this comment

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

It is an interesting question what the default should be here.

It wastes some CPU in reactive mode using eframe, but basically anywhere else it is nice with an animation.

In the future we may want to have a global egui option for "do you want animations or not?", but for now... either false or true work for me to be honest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The animation now plays on hover, I'll leave it up to you to decide whether it should be animated by default.

egui/src/widgets/progress_bar.rs Outdated Show resolved Hide resolved
visuals
.override_text_color
.unwrap_or(visuals.selection.stroke.color),
);
Copy link
Owner

Choose a reason for hiding this comment

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

This can easily overflow the size of the progress bar, which would look pretty ugly.

I think we either should either:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, the text now gets clipped in 1d0d86f.

ui.add(doc_link_label("ProgressBar", "ProgressBar"));
let progress = *scalar / 360.0;
let progress_bar = egui::ProgressBar::new(progress)
.text(format!("{}%", (progress * 100.0) as usize))
Copy link
Owner

Choose a reason for hiding this comment

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

This seems like something so common that we should have a helper for it, e.g. .show_percentage()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in 1d0d86f

@EmbersArc EmbersArc requested a review from emilk June 28, 2021 15:35
Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Wonderful - thank you!

@emilk emilk merged commit 89cea7a into emilk:master Jul 1, 2021
@EmbersArc EmbersArc deleted the progress-bar branch October 3, 2021 11:04
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.

Progress bars
3 participants