-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feat: Add progress bar #163
base: main
Are you sure you want to change the base?
Conversation
Hi @pepicrft, can you check if I’m on the right track? This is my first time creating a CLI, and I’m really excited to get your feedback |
Wow! This is amazing @M7md-Ebrahim. Thanks a lot for taking the lead here. There's no deadline here, so no rush, it's important that you take your time to land a great first contribution and get familiar with all the CLI concepts and ideas. |
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.
The animations looks amazing, great job!
I have two minor comments on the style of the animation:
- The labels and the loading blocks should have vertical center alignment
- The percentage label should be filled with spaces if less than one hundred, so the pipe
|
doesn't move as the percentage grows.
The implementation overall looks good to me – let's add some tests and documentation and I'd be happy to take another look.
Also, sorry for the PR review having taken a bit longer here. We'll try to do a better job at that, so you get a faster turnaround time 🙂
|
||
class CLIProgressBar { |
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.
class CLIProgressBar { | |
struct CLIProgressBar { |
We prefer using structs whenever possible. Let's use classes only when you need in-place mutability.
@@ -111,6 +111,15 @@ public protocol Noorable { | |||
/// - Parameters: | |||
/// - alerts: The warning messages. | |||
func warning(_ alerts: WarningAlert...) | |||
|
|||
/// |
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.
Let's add some documentation
func startProgress(total: Int, interval: TimeInterval?, block: @escaping (String, Int) -> Void) | ||
func stop() | ||
} | ||
class DefaultProgressBar: ProgressBar { |
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.
nit:
class DefaultProgressBar: ProgressBar { | |
final class DefaultProgressBar: ProgressBar { |
This PR introduces an initial implementation of a Progress Bar Component #154
Tasks:
Notes: