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

Added field_label_serializable #2399

Merged

Conversation

BeksOmega
Copy link
Collaborator

@BeksOmega BeksOmega commented Apr 22, 2019

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide

The details

Resolves

Work on #1623
Extension of PR #2389

Proposed Changes

Adds a field_label_serializable field. Which is a label field that can be modified programmatically and saved to XMl.

Also adds an option to the block factory to turn a field_label into a field_label_serializable.
LabelSerializable_Blockfactory

I can also change it to be a separate block if you think that makes more sense.

Reason for Changes

Makes it so developers don't have to create a serializable label field themselves, they can just use the prebuilt one.

Test Coverage

Added an XML test to the mocha to make sure it serializes properly.
LabelSerializable_Passing

I also added a test block to the playground with a button to help test the serialization:
LabelSerializable_Randomization

I also tested that blockfactory generates correct json and javascript.

JSON, not serializable:

{
  "type": "block_type",
  "message0": "test text",
  "colour": 230,
  "tooltip": "",
  "helpUrl": ""
}

JSON, serializable:

{
  "type": "block_type",
  "message0": "%1",
  "args0": [
    {
      "type": "field_label_serializable",
      "name": "TESTNAME",
      "text": "test text"
    }
  ],
  "colour": 230,
  "tooltip": "",
  "helpUrl": ""
}

JavaScript, not serializable:

Blockly.Blocks['block_type'] = {
  init: function() {
    this.appendDummyInput()
        .appendField("test text");
    this.setColour(230);
 this.setTooltip("");
 this.setHelpUrl("");
  }
};

JavaScript, serializable:

Blockly.Blocks['block_type'] = {
  init: function() {
    this.appendDummyInput()
        .appendField(new Blockly.FieldLabelSerializable("test text"), "TESTNAME");
    this.setColour(230);
 this.setTooltip("");
 this.setHelpUrl("");
  }
};

Tested on:

  • Desktop Chrome

Additional Information

@BeksOmega BeksOmega force-pushed the feature/LabelSerializableBlockfactory branch from 1c74966 to c2f032e Compare April 23, 2019 19:46
Copy link
Contributor

@RoboErikG RoboErikG left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I'd prefer this be a separate block in the factory for two reasons:

  1. Modifying the existing block means you need to test migration paths (loading old block definitions).
  2. It's something else developers need to think about even if they aren't using serializable labels.

* @license
* Visual Blocks Editor
*
* Copyright 2012 Google Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

2019



/**
* Class for a variable getter field.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: update comment

@BeksOmega BeksOmega force-pushed the feature/LabelSerializableBlockfactory branch 2 times, most recently from 39f7b66 to 4bfb6c6 Compare April 23, 2019 22:42
@BeksOmega
Copy link
Collaborator Author

Changed & retested code generation.

Copy link
Contributor

@RoboErikG RoboErikG left a comment

Choose a reason for hiding this comment

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

Last comment then lgtm.

@@ -4,7 +4,7 @@
<meta charset="utf-8">
<meta name="viewport" content="target-densitydpi=device-dpi, height=660, initial-scale=1.0, maximum-scale=1.0, user-scalable=no" />
<title>Blockly Demo: Blockly Developer Tools</title>
<script src="../../blockly_compressed.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, missed this. Please upload a rebuilt compressed file instead of swapping it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Gah I meant to do that!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the new field_label_serializable isn't required by any other scripts at the moment, which I think is making it so closure doesn't include it in the compressed file. Should I try to export it? or is there a different solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to add a goog.require for the new field type to blockly.js.

Side note: You can see a require for FieldDate commented out in that file which is what keeps it from being included in Blockly by default since the date picker is a relatively large dependency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rebuilt and retested!

@BeksOmega BeksOmega force-pushed the feature/LabelSerializableBlockfactory branch from 97e5b39 to 28b6e37 Compare April 24, 2019 17:36
@BeksOmega BeksOmega force-pushed the feature/LabelSerializableBlockfactory branch from 28b6e37 to e42df08 Compare April 24, 2019 17:38
@RoboErikG RoboErikG merged commit ce816b9 into google:develop Apr 24, 2019
@BeksOmega BeksOmega deleted the feature/LabelSerializableBlockfactory branch April 24, 2019 21:54
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.

None yet

2 participants