andrewhathaway / Winterfell

Generate complex, validated and extendable JSON-based forms in React.
http://winterfell.andrewhathaway.net
MIT License
785 stars 116 forks source link

String vs. number mismatch #75

Closed Teelevision closed 7 years ago

Teelevision commented 7 years ago

I noticed problems when using the checkboxOptionsInput (and maybe with others, too) and defining your options using numbers (not numbers in strings). When checking a checkbox, the value that is registered is the number as string. This seems to lead to multiple problems:

  1. The checkbox is not rendered checked.
  2. The questionsAnswers holds duplicate values if the checkbox was clicked multiple times.
  3. The isAllIn validation does not validate if given as numbers as well.

Workaround Not using numbers or convert numbers to strings in the schema.

Why I think this is a bug This library aims to provide "JSON-based forms in React". The JSON format allows numbers, so the expected behaviour would be that numbers work as options.

Possible solutions The source of this problem could be that there is no distinction between numbers and strings in HTML and that the options are used as value of the checkbox. Simply converting numbers to strings would fix most of the issues. But it would not allow that for example 42 and "42" could both be values. So another solution would be to use proxy-options as values for the checkboxes and then map the proxy-options to the real options.

Example broken question

{
    "questionId": "example-broken-question",
    "question": "Please select any number of elements",
    "input": {
        "type": "checkboxOptionsInput",
        "required": false,
        "options": [
            {
                "text": "Option one",
                "value": 1
            },
            {
                "text": "Option two",
                "value": 2
            },
            {
                "text": "Option three",
                "value": 3
            }
        ]
    },
    "validations": [
        {
            "type": "isAllIn",
            "params": [
                [1, 2, 3]
            ]
        }
    ]
}
andrewhathaway commented 7 years ago

Fixed this in #78. Will push a version very soon.