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

Checkboxgroup is not working as expected in the SmartForm #1998

Closed
eddielisands opened this issue Jun 13, 2018 · 11 comments
Closed

Checkboxgroup is not working as expected in the SmartForm #1998

eddielisands opened this issue Jun 13, 2018 · 11 comments

Comments

@eddielisands
Copy link

I tried with the "example-forum" in the latest "Vulcan-Starter" repo, I could add the new category, but could not remove the checked category from the "Edit Post" after submitting the form.
screen shot 2018-06-13 at 10 40 31

@SachaG
Copy link
Contributor

SachaG commented Jun 13, 2018

OK, I see the issue now. It's a bit tricky to fix, I need to think about it…

@Apollinaire
Copy link
Member

@SachaG So where does this bug come from ? Did it appear with the Smartform rework from last month ?
I'm going to start using groups on my end so I could have some time to look into this too

@SachaG
Copy link
Contributor

SachaG commented Jun 18, 2018

Yes it was because of the rewrite. You can see how I fixed it in the linked commit.

@eddielisands
Copy link
Author

The label of the checkboxgroup is disappeared, and the 'null' value causes the validation error if the data type is 'String'.

@Apollinaire
Copy link
Member

Yes, I can confirm that it's broken on the current devel. It works on the master branch though (v1.11.2). I don't know what could have broken this, I did not read the latest commits
And the label is missing too.

@eddielisands
Copy link
Author

eddielisands commented Jun 21, 2018

Add < label >{inputProperties.label}< /label > to the beginning of the < div >< /div > could add the label back.
The default value for the 'checkboxgroup' is [""], and it will update to [undefined, "value"] if you toggle the second checkbox. The "undefined" value breaks the graphQL validation when submitting the smart form.

SachaG added a commit that referenced this issue Jun 24, 2018
@SachaG
Copy link
Contributor

SachaG commented Jun 24, 2018

I fixed the missing label. The null value thing is trickier, let me think about it…

@SachaG
Copy link
Contributor

SachaG commented Jun 24, 2018

So the root issue is deciding whether the array containing the checkbox values can be "sparse" or not, i.e. ['foo', null, 'bar'] vs ['foo', 'bar'].

With nested forms (like in the address example) it's important to preserve empty array items because we rely on paths to update other items – so if we delete the first item, we don't want the foo.0 path to start pointing to the previously second –and now first– item.

For checkboxes though it makes less sense, so maybe we shouldn't be using that same system at all and just handle the entire checkbox array's state inside the checkbox group component instead.

SachaG added a commit that referenced this issue Jun 25, 2018
@SachaG
Copy link
Contributor

SachaG commented Jun 25, 2018

That should hopefully fix it. It was actually a lot simpler than I thought (assuming it actually works now!).

@SachaG SachaG reopened this Aug 5, 2018
@SachaG
Copy link
Contributor

SachaG commented Aug 5, 2018

Actually this is still not fixed because when going from ['a', 'b', 'c'] to ['a', 'c'] the old a new values get merged meaning you still end up with ['a', 'b', 'c']

@SachaG
Copy link
Contributor

SachaG commented Aug 8, 2018

OK now I think it works!

@SachaG SachaG closed this as completed Aug 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants