-
Notifications
You must be signed in to change notification settings - Fork 618
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
feat: Allow configuration of buttons, pixel colors, and fixed field height for Bitmap field #2351
feat: Allow configuration of buttons, pixel colors, and fixed field height for Bitmap field #2351
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sending us this. Overall it looks great and we're looking forward to accepting your changes. I have suggested a few tweaks that I think would improve things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of minor doc corrections (plus I'll need to do some local manual testing) before merging.
Blockly.Msg['BUTTON_LABEL_RANDOMIZE'] = 'Randomize'; | ||
Blockly.Msg['BUTTON_LABEL_CLEAR'] = 'Clear'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn't noticed this previously. I think it's fine as it is for the moment, but the team should probably make some decisions about how we will deal with localisation of plugins now that more formerly-core functionality has been moved out of Blockly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this, I was following guidance from @maribethb who pointed out that this is how it’s done elsewhere:
https://google.github.io/blockly-samples/plugins/block-plus-minus/README
Blockly.Msg['PROCEDURE_VARIABLE'] = 'variable:'; |
Naturally, I’m open to changing this if there’s a better way. I had originally set up to take in the translation strings as configuration options (which is why there was ‘buttonOptions’ initially).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Her guidance is certainly good. I've no issue with the code; my comment was more as a reminder to myself that we should probably think about whether we want to add messages like this to the set that get translated by translatewiki. Until now we've only had messages in core Blockly and Blockly Games translated, not messages for plugins—but with some formerly-core fields and blocks now being moved to plugins we should probably revisit that.
fix: typo in documentation Co-authored-by: Christopher Allen <[email protected]>
fix: update property name in documentation Co-authored-by: Christopher Allen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to go ahead and approve this and then merge it, because I'm satisfied that these changes do not create any significant issues, and are strictly an improvement that I know that we want to include in today's blockly-samples release.
In local testing with the supplied test blocks, however, I have noted what appears to be a bug with sizing the pop-up for the "Static field height, custom colors, no buttons" test block:
(This is on Chrome v124 on MacOS Ventura. It may be an issue with Blockly v11, which I had npm link
ed at the time.)
Can you take a look and either submit a fix or (if that is not possible for some reason) file an issue to remind us to have a look again once the dust settles on the Blockly v11 release?
@cpcallen I've prepared a follow-up PR that addresses the issue you found and one other. The pop-up sizing bug was present before this work was done as Core Blockly sets a maximum height on dropdown editors. I also proposed conditionally removing a bottom margin from the editor if no buttons are present. |
@mikeharv: wonderful, thanks! |
The basics
The details
Resolves
Resolves #2353
Proposed Changes
Blockly.Msg
system to translate button text ("Randomize" and "Clear")Examples
Hiding the "Randomize" button:
Setting a static field height for a larger image:
Customizing pixel colors:
Reason for Changes
Button text should be translatable as a best practice for international and non-English users.
Buttons should be option in the event that "Randomize" or "Clear" are not desired (ex. Code.org).
Field height should be settable in order to prevent blocks from blowing up in size with larger bitmap images.
Pixel color should be configurable so that it can be aligned to an app's block colors, external branding guidelines, or other UI considerations.
Test Coverage
The changes here affect UI and not functionality. The test blocks in the plugin playground have been updated to include a set with one hidden button, custom pixel colors, and a custom static field height.
Documentation
I updated the plugin's ReadMe to explain the new configuration options. I'm not aware of other documentation specific to this plugin.
Additional Information
This plug-in will be highlighted at the Blockly Summit in June as part of my presentation. It would be great if this could get published in time for me to update our app code to use the new options!