Fixed How Fields Handle Size #2500
Conversation
BeksOmega
force-pushed
the
fixes/FieldSizeHandling
branch
from
852ec64
to
2ba3d3d
Compare
core/field.js
Outdated
| * the approximated width on IE/Edge when `getComputedTextLength` fails. Once | ||
| * it eventually does succeed, the result will be cached. | ||
| * Updates the width of the field. Redirects to updateSize_(). | ||
| * @deprecated May 2019 Use Blockly.Field.getCachedWidth() directly instead. |
There was a problem hiding this comment.
Why does this say to use getCachedWidth instead of updateSize_?
There was a problem hiding this comment.
render_ only supports text fields. updateSize_ is only called by the default render_ function. updateSize_ also only supports text fields.
If you have a text field, there is no need to override render_, and no need to call updateSize_.
If you have an image, or mixed field you need to override render_. In this case updateSize_ will not be helpful, because it only supports text fields. But if it is a mixed field, you may still want to know the size of your text, so you should call getCachedWidth().
There was a problem hiding this comment.
I see your point, but the redirect still feels wrong to me:
updateWidthandupdateSizehave no return value, but have side effectsgetCachedWidthshas a return value and no side effects
I suggest:
@deprecated May 2019 Use Blockly.Field.updateSize_() to force an update to the size of the field, or Blockly.Field.getCachedWidth() to check the size of the field..
There was a problem hiding this comment.
Sounds good, Fixed.
core/field_checkbox.js
Outdated
| @@ -91,6 +91,14 @@ Blockly.FieldCheckbox.prototype.SERIALIZABLE = true; | |||
| */ | |||
| Blockly.FieldCheckbox.prototype.CURSOR = 'default'; | |||
|
|
|||
| /** | |||
| * Used to tell if the field needs to render. Checkbox fields are statically | |||
There was a problem hiding this comment.
"Used to tell if the field needs to be rendered the next time the block is rendered. Checkbox fields are statically sized, and only need to be rendered at initialization."
^ If this is inaccurate, let's find a more accurate way to say it.
There was a problem hiding this comment.
Similar/the same wording for other places where you're setting isDirty to false.
| * image, or if the browser is not IE / Edge, this simply calls the parent | ||
| * implementation. | ||
| */ | ||
| Blockly.FieldDropdown.prototype.updateWidth = function() { |
There was a problem hiding this comment.
Why did you remove this?
There was a problem hiding this comment.
It doesn't actually get called anywhere, all of the sizing logic is handled in the image and text render functions.
BeksOmega
force-pushed
the
fixes/FieldSizeHandling
branch
from
b8c7e82
to
730dec9
Compare
BeksOmega
force-pushed
the
fixes/FieldSizeHandling
branch
from
dac1506
to
e1e746b
Compare
The basics
The details
Resolves
#2428 and some collapse/expand problems
Proposed Changes
Reason for Changes
Note that the dropdown field's updateWidth function wasn't getting called from anywhere, so it could be safely removed.
Test Coverage
I also tested collapsing and collapsing all of the default categories, and it all looks good.
Tested on:
Was not able to test on internet explorer or edge because of #2499
Additional Information
N/A