Fix bug that coerced empty scaled float value to 0

[cjcenizal]

Fixes #62035

[elasticmachine]

Pinging @elastic/es-ui (Team:Elasticsearch UI)

[alisonelizabeth]

Code LGTM. I left a comment around the actual default value we use.

[alisonelizabeth]

Maybe we should check with an ES engineer to see what a good default value might be; 1 seems a little weird to me, since it's used as a multiplier. The documentation uses 10 as an example.

Docs: https://www.elastic.co/guide/en/elasticsearch/reference/current/number.html#scaled-float-params

[sebelga]

I would leave it as 1, as multiplying by one does not do anything. The default value should be what ES uses behind the scene in case the parameter is not specified, and I think by default ES does not scale float.

[cjcenizal]

Seb's point is also what I originally had in mind (1 is a good default because it's basically a no-op). But Colin pointed out that it would be a better UX to force the user to consider the option and enter the right value for their use case. I think he's right. It's not helpful to make it easy for users to use a scaled float that doesn't take advantage of the scaling factor (a different type would be better).

Unless anyone objects, I intend to:

• Close this PR
• Update the original issue with these notes
• Update the original issue with a note that we should also validate against a value of 1 and warn the user that this isn't very useful
• Create a blocker issue that we need to update the form hook lib to allow numeric fields to be empty instead of defaulting to 0

[alisonelizabeth]

👍 sounds good. I agree that leaving it blank would be the better experience. Thanks @cjcenizal!

[sebelga]

I haven't checked, but my guess is that it is the EuiNumeric field that defaults to 0.
I would see the reason being: an empty string ("") is not a valid number, thus we render 0.

[sebelga]

It seems that the conversion from "" to 0 occurs on the field deSerializer that we have set

deserializer: (value: string | number) => +value,

// Probably needs to be

deserializer: (value: string | number) => value === '' ? '' : +value,

cc @alisonelizabeth @cjcenizal

[cjcenizal]

@elasticmachine merge upstream

[cjcenizal]

Good catch @sebelga! I've fixed this. Could you take another look @alisonelizabeth? Now the user is required to enter input:

@sebelga When you're writing the docs for the form hook lib, you might want to consider that people could find the roles of the deserializer and formatter configuration options to be unclear.

[alisonelizabeth]

This works great now. Thanks @cjcenizal!

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 01b2c99

History

• 💚 Build #38034 succeeded eb540dc

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[sebelga]

Thanks for the insight @cjcenizal, I will take it into account to clearly explain:

3 lifecycles of field value

• deSerializer: used only when initializing the field. Applied on the default value provided
• formatter: used each time the value of the field changes
• serializer: used only when accessing the form data (form.submit()). BUT, this is not currently true. The serializer is executed each time the value changes. This is a "bug" (not doing any harm) and I plan to fix it once the mappings editor got test coverage as well as the form lib. The reason is, we might have some complex logic in the serializer and we don't want this logic to be executed on each key stroke.