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

Overriding color constants does not override all blocks with given color #1648

Closed
AnmAtAnm opened this issue Feb 21, 2018 · 7 comments · Fixed by #1782
Closed

Overriding color constants does not override all blocks with given color #1648

AnmAtAnm opened this issue Feb 21, 2018 · 7 comments · Fixed by #1782
Assignees
Labels
component: library blocks issue: bug Describes why the code or behaviour is wrong type: regression

Comments

@AnmAtAnm
Copy link
Contributor

AnmAtAnm commented Feb 21, 2018

Problem statement

Overriding color constants does not override all blocks with given color.

Expected Behavior

Overriding a Blockly color constant after loading the blocks_compressed.js and before constructing a block (ideally before injecting Blockly) should affect all blocks using the color constant.

Actual Behavior

Several block types have the incorrect color, at least among the text blocks.

Steps to Reproduce

  1. Add the following lines to the top of the first <script> in the Playground (right after 'use strict'):
Blockly.Msg.TEXTS_HUE = "#193863";  // dark blue
  1. Load the Playground and open the text blocks toolbox category. Note the mix of block colors:

screen shot 2018-03-29 at 10 59 01 am

Operating System and Browser

  • Desktop Chrome
@AnmAtAnm AnmAtAnm self-assigned this Feb 21, 2018
@CleoQc
Copy link
Contributor

CleoQc commented Feb 21, 2018

For historical purposes, this used to work with the code from a year ago. I don't know when the bug was introduced but it has been working for us before.

Thanks for creating the issue for me.

Cleo

@swdunning
Copy link

After trying to change the color of the Loops and Conditionals and not being successful I was told to go here. This is also happening with me. The blocks look almost exactly the same, first half are green and second half is blue.

@swdunning
Copy link

swdunning commented Mar 21, 2018

Also, this only happens if I add both lines of code to the "start" function, only adding the first line of code does nothing and adding them to the <script> tag does nothing either....Although, I am not sure I am doing it correctly....

<script Blockly.Constants.Text.HUE = "#193863"; src="../blo....>

@AnmAtAnm
Copy link
Contributor Author

AnmAtAnm commented Mar 28, 2018

@swdunningASU: The correct form is:

<script>
'use strict';
Blockly.Msg.TEXTS_HUE = "#193863";  // dark blue

.
.
.

@AnmAtAnm
Copy link
Contributor Author

AnmAtAnm commented Mar 29, 2018

I incorrectly wrote the bug referring to the old method of setting the color. Since the move to JSON required message references, I've updated the above bug report and comments to hint at a preference of updating the Blockly.Msg constant, instead of the prior HUE constant.

A PR with all blocks using the MSG constants is forthcoming.

@AnmAtAnm
Copy link
Contributor Author

AnmAtAnm commented Apr 2, 2018

Just for clarification, the original version of this bug report suggested Blockly.Constants.Text.HUE = "#193863"; // dark blue was the correct form. This is the form that CleoQc suggested worked one year ago.

This was replaced (unfortunately without marking it as deprecated) by the Msg constants, so they could be used in JSON without making the direct JavaScript variable references (critical for the Android and iOS implementations).

This also has the benefit of adding a level of indirection that unbinds load ordering constraints between the block definitions and the constant values. The message dereferencing occurs at block creation time instead of at JavaScript/JSON load time.

PR #1749 will ensure all predefined blocks use the Msg constants at all locations.

@rachel-fenichel
Copy link
Collaborator

Fixed by #1749.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: library blocks issue: bug Describes why the code or behaviour is wrong type: regression
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants