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.

Sign up for GitHub

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

Jump to bottom

Add random spinner #47

Merged
merged 8 commits into from
Jul 18, 2020
Merged

Add random spinner #47

merged 8 commits into from
Jul 18, 2020

Conversation

sbalay
Copy link
Contributor

@sbalay sbalay commented Jul 8, 2020

Summary

I have been developing a CLI for a couple of days now and I think it is kinda fun to have a random spinner every time I use one if its commands.

I created a wrapper of cli-spinners in the CLI and developed this feature just for myself... but maybe other people also like it, so here you go.

Feel free to close the PR if you don't want this feature in the codebase :)

Alternative implementation

Instead of having it as a function cliSpinners.random(), we could use something like Object.defineProperty to have it as an attribute cliSpinners.random

@sindresorhus
Copy link
Owner

Instead of having it as a function cliSpinners.random(), we could use something like Object.defineProperty to have it as an attribute cliSpinners.random

I usually prefer a method when the return value is not deterministic, but in this case, it could be nice that .random is just another spinner. It makes it just work in packages using this, like ora.

@sbalay
Copy link
Contributor Author

sbalay commented Jul 12, 2020

@sindresorhus done! Not sure what's wrong with CI though 😕

> xo && ava && tsd
Error: Cannot find module 'lodash/fp'
Require stack:
- /home/travis/build/sindresorhus/cli-spinners/node_modules/enhance-visitors/index.js
- /home/travis/build/sindresorhus/cli-spinners/node_modules/eslint-plugin-ava/rules/assertion-arguments.js
- /home/travis/build/sindresorhus/cli-spinners/node_modules/import-modules/index.js
- /home/travis/build/sindresorhus/cli-spinners/node_modules/eslint-plugin-ava/index.js
- /home/travis/build/sindresorhus/cli-spinners/node_modules/eslint/lib/config/plugins.js
- /home/travis/build/sindresorhus/cli-spinners/node_modules/eslint/lib/config.js
- /home/travis/build/sindresorhus/cli-spinners/node_modules/eslint/lib/cli-engine.js
- /home/travis/build/sindresorhus/cli-spinners/node_modules/eslint/lib/api.js
- /home/travis/build/sindresorhus/cli-spinners/node_modules/xo/index.js
- /home/travis/build/sindresorhus/cli-spinners/node_modules/xo/cli-main.js
- /home/travis/build/sindresorhus/cli-spinners/node_modules/xo/cli.js
Referenced from: 
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:966:15)
    at Function.Module._load (internal/modules/cjs/loader.js:842:27)
    at Module.require (internal/modules/cjs/loader.js:1026:19)
    at require (internal/modules/cjs/helpers.js:72:18)
    at Object.<anonymous> (/home/travis/build/sindresorhus/cli-spinners/node_modules/enhance-visitors/index.js:3:11)
    at Module._compile (internal/modules/cjs/loader.js:1138:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1158:10)
    at Module.load (internal/modules/cjs/loader.js:986:32)
    at Function.Module._load (internal/modules/cjs/loader.js:879:14)
    at Module.require (internal/modules/cjs/loader.js:1026:19)

sbalay
sbalay commented Jul 12, 2020
View reviewed changes
test.js Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Can you mention in the API docs in the readme and index.d.ts that the random spinner is special and will return a random spinner?

@sindresorhus sindresorhus changed the title Add method to get a random spinner Add random spinner Jul 17, 2020
@sindresorhus
Copy link
Owner

done! Not sure what's wrong with CI though 😕

Don't worry about that. I'll deal with it.

@sbalay
Copy link
Contributor Author

sbalay commented Jul 18, 2020

Can you mention in the API docs in the readme and index.d.ts that the random spinner is special and will return a random spinner?

Done

sindresorhus
sindresorhus reviewed Jul 18, 2020
View reviewed changes
index.d.ts Outdated
| "grenade"
| "point"
| "layer"
| "betaWave";
Copy link
Owner

Choose a reason for hiding this comment

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

Don't do unrelated changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, just noticed them. I'll fix them in a minute!

sindresorhus
sindresorhus reviewed Jul 18, 2020
View reviewed changes
readme.md Outdated
@@ -22,7 +22,7 @@ $ npm install cli-spinners
## Usage

```js
const cliSpinners = require('cli-spinners');
const cliSpinners = require("cli-spinners");
Copy link
Owner

Choose a reason for hiding this comment

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

https://github.com/sindresorhus/cli-spinners/pull/47/files#r456775532

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@sindresorhus sindresorhus merged commit 8e14994 into sindresorhus:master Jul 18, 2020
@sindresorhus
Copy link
Owner

Thanks :)

@erezrokah erezrokah mentioned this pull request Aug 26, 2020
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sindresorhus sindresorhus sindresorhus left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sbalay @sindresorhus