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

Migrate Blockly.hueToHex to Blockly.utils.colour.hueToHex #5462

Merged
merged 4 commits into from
Sep 14, 2021

Conversation

gonfunko
Copy link
Contributor

The basics

  • I branched from goog_module
  • My pull request is against goog_module
  • My code follows the style guide
  • My code is presented in the form suggested in the module
    conversion guide
  • I have run npm test.

The details

Resolves

Part of #5208

Proposed Changes

This PR migrates Blockly.hueToHex to Blockly.utils.colour.hueToHex and updates core and demos to use the new function.

@gonfunko gonfunko requested a review from a team as a code owner September 14, 2021 20:53
@google-cla google-cla bot added the cla: yes Used by Google's CLA checker. label Sep 14, 2021
@github-actions github-actions bot added this to the 2021_q3_release milestone Sep 14, 2021
@gonfunko gonfunko merged commit 2ba51d4 into google:goog_module Sep 14, 2021
@gonfunko gonfunko deleted the huetohex branch September 14, 2021 21:13
@gonfunko gonfunko mentioned this pull request Sep 14, 2021
33 tasks
@@ -590,7 +590,8 @@ WorkspaceFactoryController.prototype.loadCategoryByName = function(name) {
}
if (!standardCategory.colour && standardCategory.hue !== undefined) {
// Calculate the hex colour based on the hue.
standardCategory.colour = Blockly.hueToHex(standardCategory.hue);
standardCategory.colour = Blockly.utils.colour.hueToHex(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a breaking change in demos, at least until we check in an updated blockly_compressed.js. I'm running into this now with my similar change, and not sure if we should a) make the change anyway which breaks the demo but hope it works whenever we do check in blockly_compressed b) check in blockly_compressed now or c) hold off on making the change to demos until after we check in blockly_compressed

we've done option a here but not sure if that was intentional and if I should continue doing that with other changes. any thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I made changes to the code demo to add JSON serialization I tested w/ compressed to make sure it worked, but didn't check it in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was looking at this from the perspective of it being easy to forget to backport these changes later, and assuming that we'll wind up needing to do a round of regression testing/fixing regardless. Happy to revert the changes to the files in demo if you think that's a better idea though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by Google's CLA checker. type: cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants