[LiveComponent] Fix expanded choices and checkboxes in forms #221
Conversation
Lustmored
force-pushed
the
fix-expanded-choices
branch
from
2e7cc8c to
653b1a5
Compare
| 'bar' => 2, | ||
| ], | ||
| 'expanded' => true, | ||
| ]) |
There was a problem hiding this comment.
What about a ChoiceType with expanded true AND multiple true (i.e. checkboxes)? Is that correctly handled? Or does the check for $child->vars['expanded'] mess up that case? I'm genuinely just not sure - I'd love it if you could verify by testing that in a real project.
There was a problem hiding this comment.
Thanks for quick review! I've followed your suggestions in other places and allowed myself to resolve them.
You are probably right about multiple option - I have missed that. But when experimenting with component with form I've found that CheckboxType was also misbehaving (haven't got enough time yet to investigate it or write a test case for bug report), therefore my intuition tells me that expanded and multiple (that expands into multiple checkboxes) isn't working right now anyways.
My intent is to spend my OSS Friday next week to investigate problems with checkboxes and either fix them or provide meaningful bug report. I can do that as part of this PR or the next one if you prefer to have at least fix for expanded merged earlier :)
There was a problem hiding this comment.
Thank you!
My intent is to spend my OSS Friday next week to investigate problems with checkboxes and either fix them or provide meaningful bug report.
This sounds wonderful. Tbh, I believe I wrote this original code that has the bug(s) in it. I knew I needed to handle the checkbox case, but I'm not certain that I dove deeply (or thought deeply enough) about all of these expanded and multiple situations. Someone needs to dive deep and look at the whole problem. If you can do that, it would be wonderful.
I can do that as part of this PR or the next one if you prefer to have at least fix for expanded merged earlier :)
Let's get everything fixed up on this PR :)
Thanks!
There was a problem hiding this comment.
I have updated PR with fixes for single checkboxes (their null value was getting lost after any update) and also expanded and multiple choice fields.
As it is right now all basic data types (except FileType, which is another story) that I've tested are working as intended.
To check out how checkbox inputs are working I used https://github.com/weaverryan/live-demo - as it is right now writing anything in registration form in e-mail first and then clicking checkbox throws in JS. With this PR it synchronizes data between frontend and backend.
To see expanded+multiple in action you can use the same registration form from my fork: https://github.com/Lustmored/live-demo
Lustmored
changed the title
Lustmored
force-pushed
the
fix-expanded-choices
branch
from
36778a3 to
a17870f
Compare
JSON -> HTML update missed build step breaking live component
Form is already submitted at this point and knows its values
|
I think I have finished all refactoring and have added enough tests to cover all cases. The I have tested this on locally modified live twig demo (it is a bit behind with changes here btw.) - my real world project usage is right now a bit too hacky (to make it work without this PR). I think I'm ready for the next wave of reviews 😀 |
| 'data' => true, | ||
| ]) | ||
| ->add('file', FileType::class) | ||
| ->add('hidden', HiddenType::class) |
There was a problem hiding this comment.
A lot of good field types included here 👍
src/LiveComponent/tests/Fixtures/templates/components/form_component1.html.twig
Outdated
Show resolved
Hide resolved
| [ | ||
| 'checkbox' => null, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
This extra variable is confusing. I believe it is because we want to ASSERT that the checkbox field equals null. But when we SEND the data to the server, we want this field to be omitted? Is that right?
If so, are you sure that's correct? With the new way of sending the data as JSON, if I have a form with an unchecked checkbox, the initial data sent to the frontend will store this value as null (and also, if I check and uncheck the box, the model will be set to null). And we DO now send null values to the Ajax calls.
There was a problem hiding this comment.
Fun fact is - in pure HTML world unchecked checkboxes would not be sent to the server at all and before parsing data as JSON live component was following those trails and it looked like perfect solution.
BUT since data is now sent as JSON and as Symfony Form component is handling nulls as unchecked state (as my updated test shows) I believe it will be simpler to just send all nulls.
This also simplifies this test case a lot by removing variable that probably only I knew why existed :)
|
Thanks @weaverryan for the review - I will try to work on it during weekend, as it seems like mostly tidying up, so shouldn't be difficult 👍 |
# Conflicts: # src/LiveComponent/assets/src/live_controller.ts
Lustmored
force-pushed
the
fix-expanded-choices
branch
from
6689e66 to
7605821
Compare
|
@weaverryan I think I have done most of your change request. I have resolved some of them to ease tracking what was done and what's left. In a few I left comments where explanation was needed or I had doubts about change suggestions 👍 |
There was a problem hiding this comment.
WONDERFUL work. You rock @Lustmored - this was a huge, complex problem and you were a joy to work with. Let's get this merged, and if we missed any edge cases, we'll find out :).
|
Thank you @weaverryan 👍 working with you and on this project is also a pleasure, so I surely will be back - I love how enabling components+live-components are architecturally in my projects already. See you around soon 😄 |
When using live component with forms that contains expanded choices list (rendered as a list of radio buttons) extracting form values returns array of nulls (one per choice), while frontend expects single value.
Therefore after calling first
updateand then trying to call update on expanded field controller throws (The property used in data-model="${propertyPath}" was never initialized. Did you forget to add exposed={"${lastPart}"} to its LiveProp?).This simple change fixes the issue. I have also added simple test case for a few basic form types.