Refactored field.init into field.initView and field.initModel #2427
Conversation
BeksOmega
force-pushed
the
fixes/InitView
branch
4 times, most recently
from
74d338c
to
97ac327
Compare
|
Note: Just realized that there needs to be a follow up to this where the fields are examined to make sure they're initializing things correctly. FieldAngle is doing some pretty wack stuff, other fields may be as well. |
core/field_checkbox.js
Outdated
| // Checkbox has already been initialized once. | ||
| return; | ||
| } | ||
| // Logging a warning about calling a deprecated method here would be |
There was a problem hiding this comment.
Can you remove this declaration of init? Since it inherits from Field it should just call the super method instead if it's not declared.
Ditto for everything else that inherits from Field that you're updating to just call the super.
|
Derp fixed. I also added package tags to all of the methods. initView could probably be protected (and renamed to initView_) but I wanted initView and initModel to be matchy-matchy. Thoughts? |
|
@rachel-fenichel this looks good to me, but can you take a quick look in case I missed anything? |
| } | ||
| Blockly.FieldCheckbox.superClass_.init.call(this); | ||
| Blockly.FieldCheckbox.prototype.initView = function() { | ||
| Blockly.FieldCheckbox.superClass_.initView.call(this); |
There was a problem hiding this comment.
There's no longer anything to stop initView from happening twice. But I think that's fine, because initializing the view twice would mean duplicate svg elements and it would be pretty visible during development of a custom field.
|
Package is fine for initView. I agree that matching them is better. |
The basics
The details
Resolves
#1010
Proposed Changes
Moves init logic into an initView function. The init function now calls initView and initModel.
I also had to change the emoji testblocks category label because apparently IE just couldn't handle the beauty of it.
Reason for Changes
Firstly makes the lifecycle more clear. Secondly the abstract init() function now creates the field group, so fields that want to do custom dom structures (e.g. not include the border rect or text element) don't need to remember to create a field group named 'fieldGroup_'.
Test Coverage
Looked through test blocks and assured that everything was rendering correctly. RTL and LTR
Also ran mocha tests locally.
Tested on:
Additional Information
Here's a picture of how the flow works now.
If the field does not have a concrete init function (which for any future fields it should not!) any callers will hit the abstract init imediately and then the it'll just work how you'd expect. The sub-most class gets it's concrete initView called first and then that flows back up to the abstract class etc.