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

Right spinner: Adds an option to place the spinner on the right side of the text #52

Closed
wants to merge 3 commits into from

Conversation

stephen-bunn
Copy link
Contributor

Description of new feature, or changes

Adds a flag that places the spinner to the right of the text.

with Halo(text='Spinning...', right=True) as spinner:
    # stuff...

Spinning... ⠧

Also affects the stop_and_persist methods.

Checklist

  • Your branch is up-to-date with the base branch
  • You've included at least one test if this is a new feature
  • All tests are passing

Related Issues and Discussions

I don't think there has been any discussions or issues related to this feature.

@manrajgrover
Copy link
Owner

@stephen-bunn Hi, I'm not sure if I would like to include this feature. Any particular reason or use case where this would be needed?

@stephen-bunn
Copy link
Contributor Author

Feel free to close the pull request if you don't think it should be a feature.

One of the reasons I choose not to use halo is the missing functionality to place the spinner on the right side of the text. I simply opened a pull request instead of an issue because it seemed trivial to implement.

@manrajgrover
Copy link
Owner

@stephen-bunn It won't be as trivial as it sounds because we support ellipses for long texts. If someone uses that feature with this option, it would give undesirable results. Hence, I would love to know your use case for this feature since I cannot think of any.

@stephen-bunn
Copy link
Contributor Author

stephen-bunn commented Feb 25, 2018

Ah I see, there is no real use case for having the spinner on the right side of the text. It is just a style choice I would like to have.

Feel free to close.

@manrajgrover
Copy link
Owner

@stephen-bunn Looks like others are also interested in this feature. Are you still willing to take this one up? Let me know and I'll review. One change I would want is renaming the right keyword to placement and making it a string with default left. I'll review the code if you're willing to continue. Thanks for working on this and apologies for the delay.

@stephen-bunn
Copy link
Contributor Author

@manrajgrover sure thing, I've opened a new PR since the repo for this one has been removed.
Now the text values left and right are accepted by a parameter called placement.

Closing this PR due to revisions in #57.

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.

None yet

2 participants