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

Formidable Forms Integration #872

Merged
merged 11 commits into from
Sep 17, 2020
Merged

Formidable Forms Integration #872

merged 11 commits into from
Sep 17, 2020

Conversation

fpcorso
Copy link
Contributor

@fpcorso fpcorso commented Aug 20, 2020

Description

Adds Formidable Forms as one of the direct form integrations.

Related Issue

Issue: #750

Types of changes

Adds the new PUM_Integration_Form_FormidableForms class as well as the new formidableforms.js file.

Screenshots

Forms in drop down:
formidable-forms-example

Popup appearing after form submission via form submission trigger:
image

Cookie being set after submitting form within popup via the form submission cookie:
formidable-forms-cookie

This has been tested in the following browsers

  • Chrome
  • Firefox
  • Edge
  • Safari <---Don't have a way to test this browser currently.

Checklist:

  • My code has been tested in the latest version of WordPress.
  • My code does not have any warnings from ESLint.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • All new functions and classes have code documentation.

@fpcorso fpcorso changed the title Formidable Forms Integration [WIP] Formidable Forms Integration Aug 20, 2020
@fpcorso fpcorso linked an issue Aug 20, 2020 that may be closed by this pull request
Copy link
Member

@danieliser danieliser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one issue we need to look at in depth, how to handle PHP submissions if over AJAX.

Left a few comments for simple improvements as well.

assets/js/src/integration/formidableforms.js Outdated Show resolved Hide resolved
$(document).on("frmFormComplete", function(event, form, response) {
var $form = $(form);
var formId = $form.find('input[name="form_id"]').val();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do they denote a form instance ID? If the same form is used in 2 places, does it fail like Gravity Forms does?

If not you can pull an instance ID by checking how many copies of form-1 are on the page and getting the index of the current one among those. Then ++ 1 so it is a numeric representation not an array index pointer.

Copy link
Contributor Author

@fpcorso fpcorso Sep 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danieliser What exactly is the formInstanceId? I noticed many of our form integrations do not use it so it appears to be optional. I looked at the formSubmission method but it only passes the variable to the event so not sure what it's purpose is.

I tested having two of the same form on the same page. They both submit without errors, though they both have the same HTML ID of "frm_form_1_container" on the container and the same HTML ID of "form_contact-form" on the form itself. So, when submitting, the form parameter is identical no matter which of the same forms is submitted.

None of the data passes to our event handler includes any sort of unique form instance ID. And, when submitting via AJAX, the plugin removes the form from the DOM prior to the event call, so getting the index doesn't seem to be possible.
By the time our code gets called, this is all that's in the container:

<div class="frm_forms with_frm_style frm_style_formidable-style" id="frm_form_1_container">
	<script type="text/javascript">document.addEventListener('DOMContentLoaded',function(){frmFrontForm.scrollMsg(1);})</script><div class="frm_message" role="status"><p>Your responses were successfully submitted. Thank you!</p>
</div></div>

But, the event handler is not given anything that I could use to tie back to that ID to get which exact form was submitted in this instance to do an instance index check. I can use 'frm_form_'+formId+'_container' to create the ID but that would be the same for all the same forms on the page and containers = jQuery('#frm_form_'+formId+'_container'); only ever returns the first form since it expects the ID's to be unique so the containers.length is always 1 even when there are two or three of the same form on the page.

So, I have no way to determine which one was submitted or to get an index for it on the page.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Form instance ID is an identifier for which copy of Form X was submitted.

Not all support it themselves, but CF7 and NF have built in instance IDs we use.

When they don't support it I have simply calculated it myself using the CSS id.

You have a solution here using 'frm_form_'+formId+'_container'.

Looks like this.

var form = jQuery(this); // assuming this is the form element object.
var forms = jQuery('frm_form_'+formId+'_container');
var instanceID = forms.indexOf(form) + 1;

If you need to append a data point to the forms wrapper container prior to submission so that you can retrieve that data after you can do so here: https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/site/general.js#L42-L56

Copy link
Contributor Author

@fpcorso fpcorso Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danieliser I think you may have missed two crucial parts of my comment above:

1. Formidable Forms completely removes the from the DOM BEFORE the event is triggered. So, when we get to the event handler, the form no longer exists to get the index of.

Before event is called, the two forms have the HTML looks like this:

<div class="frm_forms  with_frm_style frm_style_formidable-style" id="frm_form_1_container">
    <form enctype="multipart/form-data" method="post" class="frm-show-form  frm_js_validate  frm_pro_form  frm_ajax_submit  frm-admin-viewing " id="form_contact-form">
       <!--Fields go here -->
    </form>
</div>
<div class="frm_forms  with_frm_style frm_style_formidable-style" id="frm_form_1_container">
    <form enctype="multipart/form-data" method="post" class="frm-show-form  frm_js_validate  frm_pro_form  frm_ajax_submit  frm-admin-viewing " id="form_contact-form">
       <!--Fields go here -->
    </form>
</div>

When the event is called the HTML looks like this:

<div class="frm_forms with_frm_style frm_style_formidable-style" id="frm_form_1_container">
    <div class="frm_message" role="status">
        <p>Your responses were successfully submitted. Thank you!</p>
    </div>
</div>
<div class="frm_forms  with_frm_style frm_style_formidable-style" id="frm_form_1_container">
    <form enctype="multipart/form-data" method="post" class="frm-show-form  frm_js_validate  frm_pro_form  frm_ajax_submit  frm-admin-viewing " id="form_contact-form">
       <!--Fields go here -->
    </form>
</div>

So, using $('something').index(form) returns -1.

2. Even if the form was there to use index(form), it still doesn't work because the unique form number is in the HTML ID and jQuery only returns the first container no matter what when using jQuery('#frm_form_'+formId+'_container').

So, even if the 1. wasn't an issue, it would still only look in the first container and not all of them. When I use anything like var forms = jQuery('#frm_form_'+formId+'_container');, the forms object only has 1 element in it, regardless of if I have 1 form or 5 of the same forms.

So, if I try to use something like:

$(document).on("frmFormComplete", function (event, form, response) {
    var $form = $(form);
    var formId = $form.find('input[name="form_id"]').val();
    jQuery('#frm_form_'+formId+'_container').index($form);

I get -1 regardless of which form is submitted.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get a list of elements by ID, and since the wrapper div for the form doesn't change after the event this should work. I do believe you are correct that when using a # selector directly it will return one result, but if you use an alternative such as [id=""] you shouldn't hit that.

jQuery('[id="frm_form_1_container"]');

Copy link
Contributor Author

@fpcorso fpcorso Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danieliser First, I did not misread your initial code. If I try to use indefOf() on a jQuery object, I get:
VM1934:1 Uncaught TypeError: $(...).indexOf is not a function. I viewed your code for the WP Forms integration and you used index():
https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/integration/wpforms.js#L12

You again use index() not indexOf() in the MC4WP code too:
https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/integration/mc4wp.js#L13-L14

Since indexOf() does not work and index() is what you have used in most of the previous integrations, index() must be what you mean.

Next, inside the event handler this is the form that is submitted which doesn't exist anymore. I have naturally already tried all the basic things to test which is why I am posting to get new ideas. Since the form doesn't exist when the event handler is called, the index() always returns -1 no matter what jQuery selector I try:
jQuery('.frm_forms').indexOf(this); // TypeError: $(...).indexOf is not a function
jQuery('#frm_form_'+formId+'_container').indexOf(this); // TypeError: $(...).indexOf is not a function
jQuery('.frm_forms').index(this); // Equals -1
jQuery('#frm_form_'+formId+'_container').index(this) // Equals -1;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, brain fart for sure. Ok so .indexOf is a Vanilla JS function that finds a value in an array. .index is a jQuery function doing the same on a list of jQuery objects: https://api.jquery.com/index/

That said the code I wrote for the other 2 clearly indicates that jQuery will return more than one result for a # based selector as well. I tested those pretty thoroughly with 3-4 instances of a form and rarely submitting the first.

Ok so everything should work but from this I'm gathering that this is not an expected value.

Mind you this will only be a valid jQuery object if you are inside a jQuery based handler (events using .on() or callbacks for .each() for example).

That part aside, curious have you dumped this to console or used debugger to see what it actually represents at the time this function is called?

Looking at Formiddables documentation, you should be able to access form values after submission via the form object:

The form object includes the form element that was submitted. The response object includes the content that is displayed in the success message. This script may be placed within a form's success message or it may be loaded on the page where the form is published. It cannot go in a form's Customize HTML.

https://formidableforms.com/knowledgebase/javascript-examples/javascript-after-form-submit/

So maybe like MC4WP we need to use the form object that is passed in rather than relying on jQuery's this to be correct.

I'd console.log(this, event, form, response); to see what it holds. If you can't find some reference to which DOM node was used in all of those then I guess lets send them an email and move on for now.

In all honesty we are not using the instance ID currently, though I saw the use for it early on via something like Analytics at some point.

Copy link
Contributor Author

@fpcorso fpcorso Sep 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danieliser I had tried all versions of this vs $(this) and others last week when I first ran into the problem and commented here. I showed the outputs of the various parameters and values last week in a comment.

But, to recap:

The event handler is called triggered by the form submission on the form itself. After removing the form from the DOM, it passes an HTML element to the event handler as form.

$(document).on("frmFormComplete", function (event, form, response) {
    // event is the event
    // form is the HTML element of the form containing all elements within the form
    // response is the response of the form submission and includes success message
    /// this is the document
}

Yes, you can use the form to retrieve the form values. That's not the problem. Getting multiple elements from a jQuery selector is also not the problem. There are many ways around that.

The problem that I have been explaining every day for the last week is that there is no way to identify which container/form instance is the one that was submitted because I can not find any way to tie the data passed to the event handler back to any form since the form is removed from the DOM prior to the event being called.

All of these return -1 because THE FORM NO LONGER EXISTS IN THE DOM:

$('#frm_form_'+formId+'_container').index(this); // equals -1
$('#frm_form_'+formId+'_container').index($(this)); // equals -1
$('#frm_form_'+formId+'_container').index(form); // equals -1
$('#frm_form_'+formId+'_container').index($(form)); // equals -1
$('.frm_forms').index(this); // equals -1
$('.frm_forms').index($(this)); // equals -1
$('.frm_forms').index(form); // equals -1
$('.frm_forms').index($(form)); // equals -1

At this point, we absolutely need to get the beta out by tomorrow, so I am going to go ahead and proceed without instanceID but send them an email to see if they have any ideas.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so then I did completely understand it the first time, the circles came possibly from me not properly conveying my solution for that scenario which I've previously worked out.

If you need to append a data point to the forms wrapper container prior to submission so that you can retrieve that data after you can do so here: https://github.com/PopupMaker/Popup-Maker/blob/master/assets/js/src/site/general.js#L42-L56

It actually becomes a bit more clear what I was saying with a simple addition of a comma after "If you need to,".

In any case GForms does things nearly the same way, early on I had to tag forms presubmission with the extra data I needed.

So in this case based on all that I've seen and your test results I'd do something like this

  1. During init of our site JS check for Formidable forms on the page.
  2. If they are detected, loop over them getting unique form ID's in most efficient way.
  3. Loop over form IDs and and select all forms using that ID.
  4. For each form with ID x add a data-form-instance="x" attribute via .data('form-instance', x).

Then post submission since the container is still on the page unmodified you should be able to get that instance ID via .data('form-instance').

Sorry for the circles, I'm guessing you did not parse my small 1 liner at the end of a response as the 4+ steps outlined above, shoulda explained better.

That said this is already merged, so it will need to be a patch afterwards at this point. Will open a new issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also had mentioned sample code for how this was done for another similar scenario with this comment which for some reason landed on another issue: #750 (comment)

classes/Integration/Form/FormidableForms.php Show resolved Hide resolved
@fpcorso
Copy link
Contributor Author

fpcorso commented Aug 20, 2020

@danieliser Should have asked you to hold off on review for this one as I am having trouble even getting their forms to do any submit over AJAX at all. Their options are for loading and validating using AJAX but the submission still seems to be PHP based only. Still researching.

I will update once I have an answer and then go through the comments after making any necessary changes.

@danieliser
Copy link
Member

@fpcorso are you sure they don't upsell AJAX submissions in a pro version? That might have been a limiting factor in our integration with them before since it was required AJAX to work before.

@danieliser
Copy link
Member

And no worries, I saw it come in, was already reviewing Github

@fpcorso fpcorso merged commit 73bc152 into develop Sep 17, 2020
@fpcorso fpcorso deleted the feature/750 branch September 17, 2020 14:59
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

Successfully merging this pull request may close these issues.

Form Integration: Formidable Forms
2 participants