chelnak / ysmrr

YSMRR is a package that provides simple multi-line compatible spinners for Go applications.
MIT License
71 stars 6 forks source link

add a prefix to spinner #63

Closed KScott99 closed 5 months ago

KScott99 commented 7 months ago

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 commented 7 months ago

Very nice! Thank you for your contribution.

I'll test drive the code later today.

chelnak commented 7 months ago

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 commented 7 months ago

@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 commented 7 months ago

Nice thank you 🙏

KScott99 commented 6 months ago

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

chelnak commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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

chelnak commented 6 months ago

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

Thanks!

KScott99 commented 5 months ago

@chelnak added test cases

chelnak commented 5 months ago

Thank you! Let's merge it.