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

Lots of features #17

Open
wants to merge 141 commits into
base: master
Choose a base branch
from
Open

Lots of features #17

wants to merge 141 commits into from

Conversation

SweetMNM
Copy link

@SweetMNM SweetMNM commented Sep 2, 2019

Now i know PRs usually tackle a single bug/feature but i had some ideas for this library that i thought might be useful.

I created tests and documented in the README every new feature added.

I suggest you read the README first.
Then you might want to go commit by commit to review? i don't know. Also if you do make sure you read the description for the commits. Sorry if this is a lot.

Features added (in general):
Remove a spinner using spinnies.remove() - this was before you implemented it

The spinner animation can be changed while spinning using spinnies.setFrames();

Optionally use spinners from the cli-spinners library (it's an optional dependency)

wordwrapjs is used to break text, that way words don't get broken in the middle.

In multiline spinners each line after the first line will be indented to the prefix (see demo).

Add support for the indent option - this was before #15 was implemented... Every line in a multiline spinner will be indented by this option.

Each spinner will now have it's own instance. Spinner's can be updated from the main instance and directly from that spinner.

Statuses can be set using spinnies.status();

signal-exit library is used to bind exit event instead of just binding the 'sigint' event. Also that listener will be unbound whenever spinnies is done spinning.

os.EOL is used instead of just assuming '\n'.

A regex to match '\n', '\r' and '\r\n' is used instead of just looking for '\n'.

A different util called purgeOptions under purgeOptions.js will be used to purge any kind of options. This is more of the behind the scenes since it shouldn't effect the actual way the library works but just make it easier to purge preference.

Implement custom statuses.

Improve CI (raw) rendering instead of printing out all of the spinners only the updated spinner will be printed.

Create warn and info status (super easy with custom statuses).

Add the ability to hide spinners and show them later.

Update the demo and the demo gif.

Create spinnies.text() to only modify the text.

Create spinnies.indent() to only modify the indent.

Improve CI detection by checking for more ENV variables that are used by CI services.

Add table of contents to the README.

Add 'Features' section to the README.

Also i have an idea for a 'bind' method. Something like what you suggested in #3. You can pass it a promise, if the promise rejects it will fail the spinner, if the promise resolved it will succeed the spinner. You can also pass it an observable, next() will update the spinner's text, complete() will succeed it and error() will fail it. Similar to how listr works.

Thank you so much for creating this amazing library. I hope you find the time to review this PR and hopefully merge.

setFrames() now sets this.currentFrameIndex = 0;

Logs are a thing now.

Use: spinnies.addLog(log); to add a log.

Then just use spinnies.log() or spinnies.getLogs();
wordwrapjs library is used to break text.

Each line after the first line get's indented using the prefixLength.
breakText, indentText and getLinesLength were modified to handle the indent option.

Each line in a mutli-line text spinner will be indented as well.

See:
jbcarpanelli#14
rap2hpoutre/taskz#3
jbcarpanelli#15
If you tried to update a spinner using spinners.update(), and the status of the spinner was anything but spinning (succeed, fail, stopped, unspinnable), the spinner would automatically be thrown into spinning mode.

Reason is:
`status = status || 'spinning';`
Will make the status default to spinning if not specified.
Code is now cleaner and easier to work on.

It's now clear that the spinners instance takes care of loops, rendering, etc..

And each spinner takes care of what to render.

Also this gives us to ability to do stuff like:
const spinnies = new Spinnies();
const spinner1 = spinnies.add('spinner-1', { text: 'Hello! I am the initial text', color: 'green' });
// some code
spinnies.fail('spinner-1');
// same as
spinnies.get('spinner-1').fail();
// same as
spinner1.fail();
spinners.childFunction(spinnerName, action, ...args) - will call a function on a child spinner.
childFunction will also verify spinnerName is a string and the spinner was initialized.

Before:
  succeed(name, options = {}) {
    const spinner = this.get(name);
    spinner.succeed(options);
    return spinner;
  }
After:
  succeed(name, options = {}) {
    return this.childFuction(name, 'succeed', options);
  }
`examples/demo.js` -
Uses the syntax:
spinners.update(spinnerName, options);

`examples/demo-using-get`
Uses the syntax:
spinners.get(spinnerName).update(options);
Fix the tests that were broken by giving each spinner it's own instance.

Must of the fixes involves changing:
`spinner.optionNameHereLikeStatus`
To:
`spinner.options.optionNameHereLikeStatus`

Since function now return the spinner's instance and not the spinner's options.
Mostly syntax errors.
setStatus might make the user think he is setting the spinner's status and not adding/modifying the status.

Note: statuses are still not implemented in any way shape or form.
Better support for CI and static render.

Not implementetd yet.
... To share and update statuses with all spinners and the main spinners instance.
Some parts of the docs are plans for the future (next commit), like spinner.status();
Create statusOptionsFromNormalUpdate and statusOverrides.
Write tests for purgeStatusOptions and statusOptionsFromNormalUpdate.

Fix error with statusOptionsFromNormalUpdate, where `color` is undefined.
Write tests for StatusRegistry.

Fix bug with purgeStatusOptions.
isStatic and noSpaceAfterPrefix would default to false when existing status would update.
Create `status` function.

If status is undefined or not a string the status will not be modified...
TODO: should this throw an error?

options now get passed to statusOptionsFromNormalUpdate, on update.
Write tests that test spinner modify functions with spinners.get('name').

Fix README.md indent.

Fix statusOptionsFromNormalUpdate, it now sets the textColor option for fail and succeed statuses.

statusOptionsFromNormalUpdate will run before the spinner options were purged, in spinner.update(). Which is why it will also check types.
It fixes the issue where failPrefix and succeedPrefix options will be purged and not apply for the statusOverride.

Fix the tests for statusOptionsFromNormalUpdate, to match the changes to statusOptionsFromNormalUpdate.
Tests for purgeSpinnersOptions will now check against the platformDefaultSpinner instead of defaulting to dots. This fixed the tests for platforms that don't support unicode.

In everyplace a '\n' was used it was replaced with os.EOL, fixes tests on windows + prevent future problems.
Using behaviours.test.js-expectToBehaveLikeAStatusChange to easily test for status changes.
SweetMNM and others added 30 commits September 4, 2019 22:35
breakText now takes a stream as the last argument.

stream is also passed to each spinner child so it can correctly break text.
Pass custom stream to spinnies.

For examples you could pass `process.stdout`.
Fix stream option position.
This makes it less confusing than it was before.

Instead of measuring the text without the prefix and adding the prefix length to the first line length getLinesLengh will simply map each line to it's length.
Since the `render` method no longer needs to measure the linesLength before adding the prefix it makes sense for setStreamOutput to do it. That way `render` only renders the line.
The tests for the breakText util were broken by the recent changes that required a stream to be passed to `breakText`.
Since no debug logs are logged by the spinnies instance (atm) it seems pointless to mention those methods in the readme.

Logs are still very useful for debugging spinnies when adding new features/modifying existing once.
The tests didn't consider that in ci environment the spinners don't spin and `raw mode` is used.
Fix CI link
Improve `applyStatusOverrides` by a lot.

Seriously don't even look at the code before that, it was really bad.
This reduces bundle size significantly and its a drop in replacement, meaning no code changes were required.
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.

1 participant