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

Assign colors, and then assign colors to keys #1799

Merged
merged 2 commits into from
Dec 3, 2022

Conversation

fisher-alice
Copy link
Contributor

@fisher-alice fisher-alice commented Nov 1, 2022

@code-dot-org/johnny-five was recently updated to the latest release of its upstream in this PR.

@islemaster - While implementing Code.org customizations, I noticed a small bug in lib/led/rgb.js - RGB.prototype[Animation.keys] is assigned to RGB.colors before RGB.colors is assigned to an array of strings. This PR changes the order of these lines.

jira

@fisher-alice fisher-alice marked this pull request as ready for review November 1, 2022 22:36
@islemaster
Copy link
Contributor

Cool. It looks like this regressed a couple of years ago in fbd7064 (here, to be exact).

Is this a review request? If so, will you please introduce a test to cover correct initialization of RGB.prototype[Animation.keys]?

@islemaster
Copy link
Contributor

👋 Hi, by the way! I don't think we've met, but I've appreciated the maintenance you're doing here. I'm sure you're having great fun living with the consequences of all my bad decisions 😃

@fisher-alice
Copy link
Contributor Author

fisher-alice commented Nov 4, 2022

here, to be exact

Yeah - there were a lot of updates made that day!

@fisher-alice
Copy link
Contributor Author

👋 Hi, by the way! I don't think we've met, but I've appreciated the maintenance you're doing here. I'm sure you're having great fun living with the consequences of all my bad decisions 😃

Hello and nice to meet you! I feel like I already know you from all of your PRs I've read as I've updated CDO johnny-five. I recently shared lessons learned from johnny-five, and I included a shout-out to you - see slide 10.

@fisher-alice
Copy link
Contributor Author

Cool. It looks like this regressed a couple of years ago in fbd7064 (here, to be exact).

Is this a review request? If so, will you please introduce a test to cover correct initialization of RGB.prototype[Animation.keys]?

Will do - thanks so much!

@fisher-alice
Copy link
Contributor Author

Cool. It looks like this regressed a couple of years ago in fbd7064 (here, to be exact).
Is this a review request? If so, will you please introduce a test to cover correct initialization of RGB.prototype[Animation.keys]?

Will do - thanks so much!

@islemaster - just added a unit test - thanks for the suggestion.

@islemaster
Copy link
Contributor

Looks great! Thanks!

@dtex dtex self-requested a review December 3, 2022 22:56
Copy link
Collaborator

@dtex dtex left a comment

Choose a reason for hiding this comment

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

This all looks good. Thanks for the catch and the patch.

@dtex dtex merged commit 5d5e301 into rwaldron:main Dec 3, 2022
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.

3 participants