-
Notifications
You must be signed in to change notification settings - Fork 11
Added weSerialize on the frontend. Fixes #88 #89
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
Conversation
bwmarkle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to my comment, what are the testing instructions? IE how can I see the broken state
| $.fn.extend({ | ||
| /** | ||
| * Custom jQuery serialize wrapper. | ||
| * | ||
| * When WordPress 5.6 increased the jQuery version to 3.5.1, the serialize function changed. Instead of | ||
| * sending spaces as "+", they are sent as "%20". This wrapper is for backwards compatibility. | ||
| * | ||
| * @since ### | ||
| */ | ||
| weSerialize: function() { | ||
| return $( this ).serialize().replaceAll( '%20', '+' ); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way we can not duplicate this function? Is this the 3rd copy? 1 on the backend, 2 on the frontend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test the broken state: make sure the master branch on your testing environment is up to date.
Create or use a form with a Name and Text field.
And try to submit the form. The form will not submit and F12 will show the weSerialize is not a function error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwmarkle Updated the function to be on the frontend and back end just once based on what scripts are being enqueue'd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm, weSerialize is only defined in one place?
IE the frontend-form.js script is loaded in the admin as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One will be on the frontend and one on the backend for this release.
assets/js/weforms.js
Outdated
| // /** | ||
| // * Custom jQuery serialize wrapper. | ||
| // * | ||
| // * When WordPress 5.6 increased the jQuery version to 3.5.1, the serialize function changed. Instead of | ||
| // * sending spaces as "+", they are sent as "%20". This wrapper is for backwards compatibility. | ||
| // * | ||
| // * @since ### | ||
| // */ | ||
| // weSerialize: function() { | ||
| // return $( this ).serialize().replaceAll( '%20', '+' ); | ||
| // }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bwmarkle Uncommented, this will need to stay since the weforms.js is used for weform.min.js so we will need it for this release, but will be automatically when we create the one script for the front and backend.
| $.fn.extend({ | ||
| /** | ||
| * Custom jQuery serialize wrapper. | ||
| * | ||
| * When WordPress 5.6 increased the jQuery version to 3.5.1, the serialize function changed. Instead of | ||
| * sending spaces as "+", they are sent as "%20". This wrapper is for backwards compatibility. | ||
| * | ||
| * @since ### | ||
| */ | ||
| weSerialize: function() { | ||
| return $( this ).serialize().replaceAll( '%20', '+' ); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm, weSerialize is only defined in one place?
IE the frontend-form.js script is loaded in the admin as well?
assets/js/vendor.js
Outdated
| // Adding custom jQuery serialize wrapper to the frontend and backend for WeForms 1.6.7. | ||
| // GitHub ticket created to make one script applied to frontend/backend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of an additional comment block above, how about this:
/**
* Custom jQuery serialize wrapper.
*
* When WordPress 5.6 increased the jQuery version to 3.5.1, the serialize function changed. Instead of
* sending spaces as "+", they are sent as "%20". This wrapper is for backwards compatibility.
*
* @todo This function is duplicated in both the frontend and backend. Need to have the code live in
* just one location.
*
* @since ###
*/There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed blocks and added to the block with the @todo tag.
|
@bwmarkle all comments have been addressed. On the frontend the script lives in frontend-form.js and on the backend I created a new file jquery-extensions.js that is used for the backed. jquery-extensions.js is concatenated into vendor.js. and frontend-form.js is concatenated in weforms.js. This happens in the build process. For the next release in May, jquery-extensions.js can be used for both the front and backend. |
weSerialize function was not extending to the frontend scripts.
To test:
enable SCRIPT_DEBUG in wp_config and submit a form.
Scripts will be minified upon the release of the next version.