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

Another rgb pulse revision, fix unit tests, update package version #26

Merged
merged 4 commits into from
Dec 12, 2022

Conversation

fisher-alice
Copy link

@fisher-alice fisher-alice commented Dec 8, 2022

This PR does the following:

  • adds one more revision to the Rgb class pulse method
  • fixes unit tests in test/rgb.collection.js and test/rgb.js files
  • adds new unit tests fro Rgb class pulse method
  • updates the version of this package from 2.1.0-cdo.1 to 2.1.0-cdo.2

The update to pulse method address the situation when on() is called while the Rgb is pulsing. Beforehand, the Rgb would emit RGB values in the tween state of the pulse animation. Now the Rgb will emit the RGB values stored by prev.

Screencast video before update:

before.update.-.on.mp4

Screencast video after update:

after.update.-.on.mp4

Testing

The unit tests that tested Animation.render in test/rgb.collection.js were revised since Animation.render was updated by this PR. Instead of color(), the update method is called.
Three tests broke when grunt was run.

Screen Shot 2022-12-08 at 11 48 24 AM

These tests are now fixed and handful of new tests were added in test/rgb.js for the pulse method in the Rgb class.

Screen Shot 2022-12-08 at 11 49 47 AM

Follow-up

I posted 3 PRs to mainline johnny-five so that we can hopefully move to using mainline johnny-five in the future.
Add option to round scaled sensor values - PR
Modify Animation temporalTTL so that it is configurable - PR
Adding a pulse method to the RGB class - PR

For the first 2 PRs, the implementations were a bit different from those for the forked code-dot-org/johnny-five in that the customizations were made to be configurable.

In the case that not all Code.org customizations are approved, we could plan to create a johnny-five wrapper so that we can still use mainline johnny-five and not have to maintain a forked project.

Here is a write-up of the costs/benefits of using mainline vs maintaining forked project.
This is the original investigation in moving off the johnny-five fork.

@fisher-alice fisher-alice requested review from a team and sanchitmalhotra126 December 8, 2022 18:23
@@ -215,6 +215,7 @@ class RGB {
}
},
update: {
writable: true,

Choose a reason for hiding this comment

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

Is this needed for testing?

Copy link
Author

Choose a reason for hiding this comment

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

Yes - within this test in test/rgb.js

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.

2 participants