fix: make all components return an empty string when empty

HendrikThePendric commented 4 years ago

UPDATE:

I've updated the issue title, and scope based on extensive discussions with @ismay. The text below doesn't really describe what this PR is about very accurately at all anymore. I think this comment provides the best summary for the current state of this PR: https://github.com/dhis2/ui-forms/pull/205#issuecomment-574246239

Previous behaviour

	Has `value` attribute on `<input />`	No `value` attribute on `<input />`
`checked` is `true`	Text from `value` attribute	`true`
`checked` is `false`	`null`	`false`

Current behaviour

	Has `value` attribute on `<input />`	No `value` attribute on `<input />`
`checked` is `true`	Text from `value` attribute	`true`
`checked` is `false`	`null`	`null` (changed)

Motivation

The main problem is that the previous behaviour created a fundamental difference between:
1. An unchecked checkbox that hadn't been touched (no form value)
2. A checkbox which was first checked by the user, and then unchecked again (false)
I think both scenarios should produce identical results.
It's more consistent to have the same deselected form value regardless if the input has a value attribute.
This is better aligned with our working definition of an empty field ('' || null || undefined). For example, the hasValue validator will now still produce an error for the scenario in 1.i

ismay commented 4 years ago

I see your point. It does seem weird to me to have true and null though. What if you're submitting to an endpoint that requires either true or false. You'd have to transform the values then right?

ismay commented 4 years ago

In thinking about it, I think the previous behaviour would be what I'd prefer. true or false makes sense to me for a checkbox that's either checked or not, since they're the same type (as in boolean). And if it has a value, either the value or null makes sense to me as well (null indicating that it could have a value).

So I'm not really bothered by the difference in and of itself. Is there a reason you want to align them, something that it allows that wouldn't be possible now?

This is better aligned with our working definition of an empty field ('' || null || undefined). For example, the hasValue validator will now still produce an error for the scenario in 1.i

Maybe that just indicates that we need a different validator for that scenario, if it doesn't work with the previous example?

HendrikThePendric commented 4 years ago

You'd have to transform the values then right?

Yup... But in practice you'd have to do so anyways... Because if the user doesn't touch the the input, there will be no formValue....

HendrikThePendric commented 4 years ago

For example, the hasValue validator will now still produce an error for the scenario in 1.i

So it doesn't ignore values that weren't touched?

No the opposite way around: previously it would only show an error if the input hadn't been touched (undefined). But the checked-then-unchecked would not produce an error (false)

HendrikThePendric commented 4 years ago

Maybe that just indicates that we need a different validator for that scenario, if it doesn't work with the previous example.

Yes that is a valid point. But as I see it, my main point still remains...

HendrikThePendric commented 4 years ago

In thinking about it, I think the previous behaviour would be what I'd prefer. true or false makes sense to me for a checkbox that's either checked or not, since they're the same type (as in boolean). And if it has a value, either the value or null makes sense to me as well (null indicating that it could have a value).

So I'm not really bothered by the difference in and of itself. Is there a reason you want to align them, something that it allows that wouldn't be possible now?

I'm not 100% sure I get what you mean here exactly... Basically I mainly want to align these two things:

A checkbox that was unchecked to begin with and never touched (has value undefined because the property on the form's values object has not been created)
A checkbox that was checked and then unchecked (would produce false prior to this PR)

As I see it, a checkbox a binary thing, and prior to this PR it had three distinct states/values it could take (undefined, true, false). I don't like that.

After this fix, it can still strictly speaking take three states:

undefined (if not touched)
true or the input's value if checked
null once deselected by user

But for RFF and our hasValue validator, undefined and null are equivalent.

And undefined and false are not equivalent from those perspectives.

Does that answer your question at all?

HendrikThePendric commented 4 years ago

After discussing extensively with @ismay on Slack, we decided to take the following steps:

Ismay:

Open an issue in RFF to find out why the default behaviour for built-in form-input components isn't that consistent

Hendrik:

Broaden the scope of this issue so all component act consistently when they are empty
Await input from other colleagues and input on the RFF issue before merging

ismay commented 4 years ago

Open an issue in RFF to find out why the default behaviour for built-in form-input components isn't that consistent

See: https://github.com/final-form/final-form/issues/315

So initially the purpose of the proposed changes wasn't clear to me. After talking about it, the way I understand them is that it's meant to simplify things like conditionally doing something based on form state. Where currently you'd have to do:

if (!values.toppings || (Array.isArray(values.toppings) && values.toppings.length === 0) ) {
    //...
}

Whereas after these changes you'd be able to do:

if (!values.toppings) {
    //...
}

Since I'm not sure why FF handles form state the way it does, we opened an issue to clarify. After that we can see if we want to diverge from that or not. If diverging I think it'd be good to weigh the maintenance burden vs. the benefits of being able to use simpler conditions.

Feel free to correct me if I've misinterpreted the above @HendrikThePendric!

HendrikThePendric commented 4 years ago

I'll try summarise as briefly as I can what this PR does below:

Previously, for some of our components, there was a difference in the form state between an "untouched" input and a "cleared" input*.
This PR fixes that, it makes sure all onChange handlers send an empty string when an input is empty. In turn this will remove the RFF field's property-name from the form's values object, which is identical to the initial state.
This is a good thing, because it means the form values are consistent, and an app-developer won't have to write several clauses in if statements (see the example @ismay provided for details)

[*] With "untouched" I mean the user does not trigger onChange on the input before posting the form. And "cleared" means that the user did enter/select a value on the input, but then removes/deselects that value before posting the form.

HendrikThePendric commented 4 years ago

I had a hangouts-call with @Mohammer5 to demo the changes of behaviour implemented in this PR. During the call he asked about how the current implementation would work in tandem with initialValue, and we tried some things. It turned out that the approach implemented in this PR causes very unexpected behaviour when combined with initialValue.....

To illustrate, suppose we have a Checkbox with initialValue={false} and we post the form:

When untouched: { checked: false }
When checked: { checked: true }
When unchecked/cleared: { }

So actually, we have created a problem similar to the one we were trying to fix, only more obscure, and confusing. This was a BAD IDEA.

Closing this PR now.

dhis2 / ui-forms