-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 isSerializable and SERIALIZABLE to Field #2389
Added isSerializable and SERIALIZABLE to Field #2389
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.
We don't have mocha running on our server yet--are you running these tests locally?
core/field_angle.js
Outdated
* @type {boolean} | ||
* @const | ||
*/ | ||
Blockly.FieldAngle.prototype.EDITABLE = true; |
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.
Since EDITABLE defaults to true on fields, you don't need to redeclare it on each of these classes. You only need to declare it for classes that set it to false. The inverse is true for SERIALIZABLE.
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.
Fixed.
One lint error on travis: https://travis-ci.org/google/blockly/builds/521780332#L536 |
That console log looks appropriate and useful. What's Travis' logic that flags this console usage and not others? |
018b102
to
c123514
Compare
Not sure, actually. it should all be running with the same eslint rules, since our eslintrc is in our root directory, and that has the no-console rule disabled. Weird. |
c123514
to
b139a24
Compare
6e35f6c
to
e8573bc
Compare
Ok @rachel-fenichel I changed the mocha eslint config to extend the root one, and then I fixed the lint errors in the other mocha files. I can separate those changes into a separate PR if you like, otherwise I think this is good to go. |
Awesome, thanks for tracking that down. |
The basics
The details
Resolves
Work on #1623
Proposed Changes
Adds a SERIALIZABLE property to fields. Adds an isSerializale function that handles warning & backwards compatibility logic.
I also reorganized the test-blocks toolbox so now it looks like this:
Reason for Changes
Sometimes you would want to programmatically modify a field, but not allow users to modify it, and save it to XML. Before you would have to create a mutator for this purpose. Now you can simply use a serializable field.
Test Coverage
I added unit tests! I added tests for the isSerializable function to make sure it works in a backwards compatible way. I also added serialization tests to make sure the XML for different fields was comming out correctly.
Here's also a quick example of some fields being serialized and deserialized:
Here's the generated XML for that example:
Tested on:
Additional Information
SERIALIZABLE is false by default. This is so that it is backwards compatible with any non-editable fields that people had created in the past.
The problem with this is that if you had created a custom editable field before you'll get a warning message when it goes to serialize, because EDITABLE is true and SERIALIZABLE is not, which is an invalid state. It still serializes! But you will get that warning. I don't necessarily /like/ this but it's the best solution I could come up with.
Also I'm planning on adding field_label_serializable and field_image_serializable fields, but the field_label_serializable stuff started cluttering up this PR and the fromXML toXML change needs to get in before field_image_serializable can be added.