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

add a prefix to spinner #63

Merged
merged 3 commits into from
Jan 17, 2024
Merged

add a prefix to spinner #63

merged 3 commits into from
Jan 17, 2024

Conversation

KScott99
Copy link
Contributor

Add the ability to set the text before a spinner

For my use I wanted to be able to indent certain spinners
Screenshot 2023-11-27 at 4 30 08 PM

This seemed like it would be useful to have in general

@chelnak
Copy link
Owner

chelnak commented Nov 28, 2023

Very nice! Thank you for your contribution.

I'll test drive the code later today.

@chelnak
Copy link
Owner

chelnak commented Nov 30, 2023

Hey @KScott99, I took a look at your PR, I really like the idea.. though i found it tricky to work out how you intended it to be used.

As a first step, could you add an example program to examples/ that shows how it works?

Thanks again for your contribution.

@KScott99
Copy link
Contributor Author

KScott99 commented Dec 1, 2023

@chelnak I added two different examples, basic_with_prefix just demonstrates how to use it, indented_spinners is closer to what I use it for

@chelnak
Copy link
Owner

chelnak commented Dec 1, 2023

Nice thank you 🙏

@KScott99
Copy link
Contributor Author

@chelnak do the examples make sense? Let me know if there's anything else you need me to do

@chelnak
Copy link
Owner

chelnak commented Dec 18, 2023

Hey @KScott99, sorry for the delayed response. I've been stacked with work and some personal things!

I will re-review the changes later this evening and get back to you. IIRC I had a few suggestions but need to remind myself what they were.

@chelnak
Copy link
Owner

chelnak commented Dec 18, 2023

OK So I'm refreshed!

What do you think about having the prefix as an option for NewSpinnerManager?

For example, if we decide that we want a spinner group with a prefix, we could do something like:

sm := ysmrr.NewSpinnerManager(
	ysmrr.WithAnimation(animations.Pipe),
	ysmrr.WithSpinnerColor(colors.FgHiBlue),
	ysmrr.WithPrefix("Download"), // <- this is your option
)

Then I think the other functions you added, UpdatePrefix and UpdatePrefixf make more sense as things that can update the prefix text, rather than initialize it.

Finally, shall we change the use of "Prefix" to something like "Title"?

@KScott99
Copy link
Contributor Author

@chelnak no worries, I understand.

I don't think adding the prefix to the entire spinner group would make sense as then you wouldn't be able to set different values for individual spinners. In my example indented_spinners I dont add a prefix to the first spinner but I do add it to subsequent spinners, adding it to the entire group wouldn't work in this case.

As for renaming to "Title" I think that could possibly be misleading from a users perspective, when I think of "Title" I think of text that comes above versus "Prefix" where I think of text that comes before something

@chelnak
Copy link
Owner

chelnak commented Dec 20, 2023

Ah ok, great clarification.

I think I may have completely misunderstood an implementation detail here.

In this case what you have suggested makes perfect sense.

Can you some test cases for your change? Once this is done, I'm more than happy to merge and cut a release!

Thanks 🙏

@KScott99
Copy link
Contributor Author

@chelnak awesome, I can work on writing some test cases just may be a bit before I can get around to it

@chelnak
Copy link
Owner

chelnak commented Dec 29, 2023

No worries at all. Ping if you need help or anything.

Thanks!

@KScott99
Copy link
Contributor Author

@chelnak added test cases

@chelnak
Copy link
Owner

chelnak commented Jan 17, 2024

Thank you! Let's merge it.

@chelnak chelnak merged commit 733674d into chelnak:main Jan 17, 2024
1 check passed
@chelnak chelnak added the enhancement New feature or request label Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants