Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple issues with checkboxes #471

Open
NicoHood opened this issue Jan 7, 2021 · 6 comments · May be fixed by #472
Open

Multiple issues with checkboxes #471

NicoHood opened this issue Jan 7, 2021 · 6 comments · May be fixed by #472
Labels

Comments

@NicoHood
Copy link
Contributor

NicoHood commented Jan 7, 2021

I recently opened PR #470 which tries to fix the default states for checkboxes. I found more issues and it turnes out this is even more complex, so I will try to explain:

Current issues:

  1. use: keys is swapped: https://github.com/getgrav/grav-plugin-form/blob/develop/templates/forms/fields/checkboxes/checkboxes.html.twig#L20
    The documentation says:

When set to keys, the checkbox will store the value of the element key when the form is submitted. Otherwise, it will use the element value.

So from my understanding the logic must be changed. However that is not the biggest issue, it just makes it harder to understand.

  1. Submitting the keys (currently use: null) and specifying default values is broken and I do not know how to fix this. Let me explain why using this example:
my_field:
    type: checkboxes
    label: A couple of checkboxes
    default:
        option1: true
        option2: false
    options:
        option1: Option 1
        option2: Option 2

With the current code, option2 is marked as checked. This is because {% set checked = (field.use == 'keys' ? value[key] : key in value) %} will look for key in value. But on a new page load value is filled by field.default which is formatted like this: "option1" => true. value[key] would be correct to use here.

If you now submit the form you will get a different layout of value: "my_field-option1" => option1. Now it would make sense to use key in value instead.

In order to fix this issue we need to know, if value was taken from the defaults or from the form submission. That is possible via {% set realvalue = form ? form.value(field.name) : data.value(field.name) %} or other methods. However what I did not (yet) found possible was to tell if a form was submitted with all checkboxes turned off. A fresh page reload always gives a value of null, but the realvalue from the snippet before will also return null (instead of an empty array).

I assume (but dont know exactly yet) that this is due to the http/form protocol, that is not sending anything to the server, if all checkboxes are empty. We would need to know somehow if a form was already submitted or the page was reloaded with a fresh value. And I have not considered any remember option here...

This is quite tricky, I hope this explained it good enough.

@NicoHood
Copy link
Contributor Author

NicoHood commented Jan 7, 2021

Just one thought: Why do we even need the use: null option? Can't we make use: keys option the default (even though the name is wrong in my opinion)? This means data is always sent via option1=1 and the issue can be resolved easier.

@mahagr
Copy link
Member

mahagr commented Jan 8, 2021

Without use: keys the default and value will be like this:

my_field:
  - option1
  - option2

With keys:

my_field:
  option1: 1
  option2: 1

Default is the value as it would be in the saved YAML file. I cannot see any bug in this. If you copy the saved value into default, it works.

@mahagr mahagr added the question label Jan 8, 2021
@mahagr
Copy link
Member

mahagr commented Jan 8, 2021

What comes to sending all the values unchecked, yes, it will revert back to the default value because of limitations in the checkbox field in HTML. However, Grav does support detecting this on some level (frontend forms + flex objects in admin), but I'm not 100% sure if the logic is to unset the value or make it empty. I bet on the wrong behavior.

@NicoHood
Copy link
Contributor Author

NicoHood commented Jan 8, 2021

But then the documentation is invalid:
https://learn.getgrav.org/16/forms/forms/fields-available#checkboxes-field
--> PR: getgrav/grav-learn#889

The fields should not be reset to the defaults. I can test and fix that, if you give me any hint on how to detect a form submission vs fresh page load. Edit: I found the correct place, I will work on a fix... https://github.com/getgrav/grav-plugin-form/blob/develop/classes/Form.php#L841-L843

@NicoHood
Copy link
Contributor Author

NicoHood commented Jan 8, 2021

A possible solution would be:

https://github.com/getgrav/grav-plugin-form/blob/develop/classes/Form.php#L844

if ($field['type'] === 'checkboxes' && !isset($data[$name])) {
    $data[$name] = [];
}

https://github.com/getgrav/grav-plugin-form/blob/develop/templates/forms/fields/checkboxes/checkboxes.html.twig#L3

{% set originalValue = value %}
{% set value = (value is null ? field.default : value) %}
{% if field.use == 'keys' and field.default and value is null %}
    {% set value = field.default|merge(value) %}
{% endif %}

It is very similar how checkboxes work. An empty array [] can be differentiated from null and the code now works with keys and without.

==> PR: #472

NicoHood added a commit to NicoHood/grav-learn that referenced this issue Jan 8, 2021
rhukster pushed a commit to getgrav/grav-learn that referenced this issue Jan 8, 2021
NicoHood added a commit to NicoHood/grav-plugin-form that referenced this issue Jan 9, 2021
@NicoHood NicoHood linked a pull request Jan 9, 2021 that will close this issue
@mahagr
Copy link
Member

mahagr commented Jan 11, 2021

PS. this doesn't fix admin which doesn't use form plugin (except in Flex Objects, though it uses different code path, too).

NicoHood added a commit to NicoHood/grav-plugin-form that referenced this issue Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants