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

Added colors for spinners #32

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Added colors for spinners #32

wants to merge 5 commits into from

Conversation

ad4mx
Copy link

@ad4mx ad4mx commented Jul 18, 2022

Added colors for spinners. I didn't really like the termcolor library you recommended, and instead went with a simpler variant. Everything is tested and examples are updated.

@ad4mx ad4mx marked this pull request as ready for review July 18, 2022 08:16
@@ -8,6 +8,7 @@ fn main() {
let mut sp = Spinner::new(
Spinners::from_str(&spinner_name).unwrap(),
"Waiting for 3 seconds".into(),
None,
Copy link
Owner

Choose a reason for hiding this comment

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

@ad4mx can we keep our backward compatibility and make the parameter color with a default none value? so the upgrade of spinners will be painless?

Copy link
Author

Choose a reason for hiding this comment

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

As far as I know, this is impossible in Rust, because you have to supply all needed arguments. I'll look more into it.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, I think moving it to a dedicated function would be a better idea then. I've also edited the code according to clippy.

/// ```
pub fn with_timer_and_stream(spinner: Spinners, message: String, stream: Stream) -> Self {
Copy link
Owner

@FGRibreau FGRibreau Aug 7, 2022

Choose a reason for hiding this comment

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

the issue I'm having here is the cardinality of spinners crate options. For instance someone might want to have both a special stream AND a customized color.

While stream should be set everywhere, I think "color" is one special use-case of text customization and should be customizable through a text mapping function you can optionally set while creating a spinner.

Then we could update spinner create documentation to explain how to manage text colors.

From a library standpoint it makes way more sense don't you think?

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