silverstripe / silverstripe-userforms

UserForms module provides a visual form builder for the Silverstripe CMS. No coding required to build forms such as contact pages.
BSD 3-Clause "New" or "Revised" License
134 stars 226 forks source link

Multiple Forms #543

Closed nglasl closed 5 years ago

nglasl commented 7 years ago

This is more an edge case, but when you happen to have two UDFs on a single page, the JS isn't bound properly to each one (since it only ever assumes there's going to be one). I basically found that the fields on the second form would be hidden, and even when submitted, the submission actually went through to the first form.

The temporary solution was to block UserForm.js completely, which basically just means you can't display steps for your UDFs (among other minor things).

dhensby commented 7 years ago

I don't think multiple UDFs on one page is a supported use-case. How is this possible within the bounds of the current module?

nglasl commented 7 years ago

It's always good to assume that multiple forms may exist, as with any other element when you're writing JS. The use case that I'm hitting is a newsletter type page, which basically outputs the content from children pages when viewing the parent, and this one just so happens to include two UDFs. The JS is the only issue here, otherwise it works perfectly.

dhensby commented 7 years ago

I completely agree the JS should be written to work that way - but I'm wondering if the fact that it doesn't is actually a bug.

Aren't there many other challenges - as you point out submitting the second form actually goes to the first... It's just not something this module is designed to handle.

In reality as nice as it would be to let it work with many on one page, is this more of a feature request than a bug?

nglasl commented 7 years ago

When submitting the second form, the submission actually going through to the first form is caused by how the JS is being bound. As I mentioned, when you block UserForm.js, there are no longer any problems and each UDF functions correctly, independent of the other.

I wouldn't say it's a feature request, but considering the use case is quite specific, it seems more like a low priority issue to me. At the least, this is bound to help someone that's going to hit the same problem.

dhensby commented 7 years ago

Please can you confirm the release version that you're using/is affected?

nglasl commented 7 years ago

This is using the 4.0.0 release, however I have tested and confirmed that the problem still exists when using the latest commit on master.

3Dgoo commented 7 years ago

Having multiple user defined forms on one page also has an issue in that every form has the same id html attribute value of UserForm_Form. This causes invalid html.

We could change the form name from "UserForm_Form" to "UserForm_Form" . $this->ID". This would make the form ID unique for every form and fix that issue.

I'm happy to submit a pull request with this fix if you would like me to do so. Note, this wouldn't fix the JavaScript issue mentioned above.

dhensby commented 7 years ago

@3Dgoo happy to accept improvements of that kind but I feel that would need to be in master (the next major release) as people may rely on the ID for their own JS...

The real issue is fixing our JS and I had a look at it and I'm not going near it ;)

I think I'm going to close this as a feature request - if someone wants to work on it I'll accept a patch that allows this to happen, but it's not currently a use case we're supporting.

3Dgoo commented 7 years ago

Sounds good. Will do.

robbieaverill commented 6 years ago

This feature is now something that this module should support, so reopening.

robbieaverill commented 6 years ago

Issue linked to: https://github.com/dnadesign/silverstripe-elemental/issues/176

kinglozzer commented 5 years ago

Re-opening - this is now functional, except with multiple pages due to selectors like $('.step-navigation') instead of this.$el.find('step-navigation'). Will open a PR with my semi-fudged solution

robbieaverill commented 5 years ago

Closing with the merge of #854