Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

spinners: fix spinner color issues #5001

Merged
merged 1 commit into from
Apr 21, 2022
Merged

Conversation

benjamincburns
Copy link
Contributor

@benjamincburns
Copy link
Contributor Author

benjamincburns commented Apr 7, 2022

Before this can be brought out of draft status, I need to:

  • Get Truffle improvements spinnies#1 approved and merged
  • Needs a code review ideally, though that doesn't necessarily need to block this
  • Release & publish the @trufflesuite/spinnies package
  • I don't believe that I have the necessary rights in the trufflesuite NPM org to do this
  • Update packages/spinners/package.json to use the properly released @trufflesuite/spinnies package

Now that I've had a green build, I'll preemptively push the updated package.json file. Unfortunately this will cause the build to go red until the package mentioned above is published.

@benjamincburns benjamincburns changed the title spinners: change to truffle spinnies, fix small color issues spinners: fix spinner color issues Apr 7, 2022
@benjamincburns benjamincburns force-pushed the more-spinner-improvements branch 3 times, most recently from 3ae4f19 to 96a2e05 Compare April 13, 2022 07:06
@benjamincburns benjamincburns marked this pull request as ready for review April 13, 2022 07:07
Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

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

Mostly makes sense to me. Only one line I don't quite understand.

@@ -12,7 +12,7 @@
"bugs": {
"url": "https://github.com/trufflesuite/truffle/issues"
},
"version": "0.1.2",
"version": "0.2.0-0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, interesting, actually changing this in the PR itself... we haven't typically done this, but it might be a good idea for us to start!

@@ -45,55 +49,54 @@ export class Spinner {
return;
}
spinnies.remove(this.name);
spinnies.checkIfActiveSpinners();
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, as someone not familiar with all the spinner changes, what's the significance of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The line that was removed was a workaround for a known issue in the official spinnies module. I fixed that issue in our fork, and removed the workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug in question was: jbcarpanelli/spinnies#34

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks!

@haltman-at haltman-at self-requested a review April 21, 2022 00:50
@benjamincburns benjamincburns merged commit c1a8eb4 into develop Apr 21, 2022
@haltman-at haltman-at deleted the more-spinner-improvements branch April 22, 2022 00:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants