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 .isSpinning property #73

Merged
merged 3 commits into from Apr 30, 2018
Merged

Add .isSpinning property #73

merged 3 commits into from Apr 30, 2018

Conversation

xavdid
Copy link
Contributor

@xavdid xavdid commented Mar 7, 2018

Thought about expanding isSpinning to be

return this.enabled && this.id !== null;

since it can't be spinning if it's not enabled, but wasn't sure about the implications. That would simplify the check in start() to be if (this.isSpinning)

@SamVerschueren
Copy link
Contributor

SamVerschueren commented Mar 8, 2018

I'm wondering, if we name it isSpinning, shouldn't we make it a method instead? Either I think we should go with ora.spinning or ora.isSpinning().

@sindresorhus
Copy link
Owner

sindresorhus commented Mar 10, 2018

@SamVerschueren Why can only methods have the is-prefix? I always go with is for booleans no matter if they're properties or methods. I prefer properties for things that don't have side-effects or require a lot of computation. That's my general rule.

@SamVerschueren
Copy link
Contributor

SamVerschueren commented Mar 10, 2018

I'm fine with either though, I think it's more personal preference. Which is the case in programming almost all the time :).

@xavdid
Copy link
Contributor Author

xavdid commented Mar 11, 2018

Cool! Name aside, is the code reasonable? Anything that I need to change?

Copy link
Contributor

@SamVerschueren SamVerschueren left a comment

Needs some docs to. Code looks good.

@sindresorhus sindresorhus changed the title add .isSpinning Add .isSpinning property Mar 14, 2018
@xavdid
Copy link
Contributor Author

xavdid commented Mar 16, 2018

@SamVerschueren updated!

readme.md Outdated
@@ -131,6 +131,10 @@ Stop the spinner, change it to a yellow `⚠` and persist the current text, or `

Stop the spinner, change it to a blue `ℹ` and persist the current text, or `text` if provided. Returns the instance.

#### .isSpinning

Returns a boolean that describes whether the instance is currently spinning.
Copy link
Contributor

@SamVerschueren SamVerschueren Mar 17, 2018

Choose a reason for hiding this comment

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

By using the Returns word, it looks like it's a function returning something. Should we rather use something like

Retrieve the status of the spinner.

Short but describes what it does.

Or just leave it as is. This is really nitpicking :).

Copy link
Contributor Author

@xavdid xavdid Mar 31, 2018

Choose a reason for hiding this comment

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

Especially because the other functions end in parens, it's clear to me that this is a property not a function. Happy to change if you want, but if it were my choice I'd leave it. ¯\_(ツ)_/¯

@sindresorhus sindresorhus merged commit 3df8d0d into sindresorhus:master Apr 30, 2018
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

3 participants