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

feat: loading spinner #93

Merged
merged 4 commits into from
Mar 9, 2022
Merged

feat: loading spinner #93

merged 4 commits into from
Mar 9, 2022

Conversation

berendsliedrecht
Copy link
Member

@berendsliedrecht berendsliedrecht commented Mar 8, 2022

  • See video in slack for implementation

closes #93

Signed-off-by: Berend Sliedrecht [email protected]

Signed-off-by: Berend Sliedrecht <[email protected]>
Copy link
Contributor

@jl-animo jl-animo left a comment

Choose a reason for hiding this comment

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

tested locally, looks really cool! had one question and suggestion

Comment on lines 22 to 26
pub fn start_loader(loader: Loader) {
match loader {
Loader::Spinner => spinner_loader(),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How does the spinner know when to stop? 😅


I have an alternate suggestion for the spinner API:

two functions: start_spinner() and stop_spinner(). Do you think something like this is possible with a reasonable amount of effort 😅 ?

Sample code:

start_spinner();
do_something_async().await;
stop_spinner();

let me know what you think!

@berendsliedrecht
Copy link
Member Author

berendsliedrecht commented Mar 9, 2022

@jl-animo I resolved your feedback. with macaronis.

HOWEVER.... it does break pipe sadly to jq for example. (I wonder if there is something for that..)

FIXED by sending the loader to stderr

@berendsliedrecht berendsliedrecht merged commit cebbd1c into main Mar 9, 2022
@berendsliedrecht berendsliedrecht deleted the feat/loading-spinner branch March 9, 2022 14:25
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.

3 participants