Fix handling of checkbox values in developer tools#2684
Fix handling of checkbox values in developer tools#2684mikeharv merged 1 commit intoRaspberryPiFoundation:mainfrom
Conversation
The string `'FALSE'` is truthy in Javascript, so Image field definitions copied from this block creator would always inadvertently have `flipRtl` set to `true`. The Checkbox field accepts both `'TRUE'`/`'FALSE'` strings and booleans so it works in practice, but the Typescript API specifies that the JSON config value should be boolean, so I added boolean conversion there too.
mikeharv
left a comment
There was a problem hiding this comment.
Hello Vilim,
I was able to reproduce this bug by creating a block with an image field that shouldn't flip for RTL. As you found, it seems that image does flip unexpectedly. Thanks for the fix!
Just a small clarification regarding the checkbox field: when the field is created, checked sets its initial value (not a config value). TypeScript enforces that this values is CheckboxBool which intentionally includes 'TRUE' | 'FALSE' | boolean.
The image field version is different (and was broken before your fix) because flipRtl is a strictly boolean config value.
Could you please resubmit with only the changes for the image field?
Look here: https://github.com/RaspberryPiFoundation/blockly/blob/cdcdaf32869cc1c04a237d416d46a9e507424bdc/packages/blockly/core/field_checkbox.ts#L248. It is fine because it later gets forwarded into a function that accepts The conversion before the constructor call in https://github.com/RaspberryPiFoundation/blockly-samples/pull/2684/changes#diff-02a72210435eaabd5c8693bf44013983f84d6b9dd981e8b0725ee278886efa9cR43 is not necessary, but in my opinion, it would be better to explicitly convert checkbox values into booleans always, to prevent mistakes like with the image field from slipping by. |
mikeharv
left a comment
There was a problem hiding this comment.
Thanks. This makes sense, and I agree there's a bit of a mismatch in the typing here.
I agree the JSON generator should convert checked to a boolean, since the JSON config interface expects a boolean there.
For the JavaScript generator, the current behavior is already valid (FieldCheckbox accepts 'TRUE' | 'FALSE' | boolean), so the conversion isn't strictly necessary. That said, I'm fine with updating it for consistency between the JSON and JS outputs.
The Checkbox field's values are not booleans but strings
'TRUE'or'FALSE'.The string
'FALSE'is truthy in Javascript, so Image field definitions copied from this block creator would always inadvertently haveflipRtlset totrue.This patch adds conversion from the checkbox string to its boolean value to ensure intended behavior.
The Checkbox field accepts both
'TRUE'/'FALSE'strings and booleans so it works in practice, but the Typescript API specifies that the JSON config value should be boolean, so I added boolean conversion there too.