Dynamic Form: boolean fields related fixes #4586
Conversation
| @@ -9,7 +9,7 @@ function orderedInputs(properties, order, targetOptions) { | |||
| name: key, | |||
| title: properties[key].title, | |||
| type: properties[key].type, | |||
| placeholder: properties[key].default && properties[key].default.toString(), | |||
| placeholder: (properties[key].default === false || properties[key].default) && properties[key].default.toString(), | |||
There was a problem hiding this comment.
If I understand this correctly, it should add placeholder for fields with default value false, right? If yes - isn't it better to use something like isNil(properties[key].default && toString(properties[key].default)?
There was a problem hiding this comment.
I'm not sure I understand your question, but basically if a default value exists, it takes the default value and converts it into a string. Previously the check would miss default value of false as it doesn't pass the check -- this change fixes this.
There was a problem hiding this comment.
My question is about (properties[key].default === false || properties[key].default). I understand why there's a check for properties[key].default === false, but second part will not match, say, number 0 (it's evaluated to false), so fields with default numeric value 0 will not have placeholder as well. So I suggested to replace that entire thing with isNil which basically is a check for value === undefined || value === null - so all fields that have defined default value will have a placeholder. There is also a case when default value is empty string, but in this case it will receive an empty string placeholder - the same as with current code.
There was a problem hiding this comment.
I don't know if numeric fields with default 0 should have a placeholder at all, so maybe it's expected behaviour and my comment does not make any sense 🤷♂️
There was a problem hiding this comment.
You're right the nil check will work better. I'll fix this.
As for the question about numbers, I'm not sure. You're probably right but I would rather not make tto many changes here as I don't fully understand the code. 😬
There was a problem hiding this comment.
Looking again at this, from what I remember this is just supposed to make any default value a placeholder in the inputs. It looks like Lodash's toString does exactly that (converts all to String, except null or undefined).
Having only this lodash function should make it a bit more readable (at list in this line 😅).
There was a problem hiding this comment.
_.toString returns an empty string for null/undefined.
There was a problem hiding this comment.
Yes, but it seemed ok to have placeholder: "" in such cases as the result should remain the same 🙂
arikfr
force-pushed
the
fix-dynamic-form-bool
branch
from
9465d88
to
19eac8b
Compare
What type of PR is this? (check all applicable)
Description
Two fixes:
falsewe would setfalseas the default value instead of the string value, which fails prop types.