Try to replace more jQuery in formidable.js#2628
Conversation
WalkthroughRefactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
js/formidable.js (4)
171-186: Clarify and hardenenableSaveDraftinput handlingAccepting both jQuery objects and HTMLElements here is good and consistent with other helpers. To make this more defensive against unexpected inputs (e.g. being passed a NodeList or
null), consider also guarding on the presence ofquerySelectorAll:function enableSaveDraft( $form ) { - const form = $form instanceof jQuery ? $form.get( 0 ) : $form; - if ( ! form ) { + const form = $form instanceof jQuery ? $form.get( 0 ) : $form; + if ( ! form || ! form.querySelectorAll ) { return; }This keeps current behaviour but avoids a possible
TypeErrorif a non-element slips through.
515-523: DOM-basedgetFileValsis fine; you can short‑circuit for clarity/perfThe new selector set and DOM usage looks equivalent to the previous jQuery version given how
fileIDis constructed. To avoid walking the whole NodeList once a non-empty value is found, you could use a simple loop with early return:function getFileVals( fileID ) { - let val = ''; - const fileFields = document.querySelectorAll( - 'input[name="file' + fileID + '"], input[name="file' + fileID + '[]"], input[name^="item_meta[' + fileID + ']"]' - ); - - fileFields.forEach( function( field ) { - if ( val === '' ) { - val = field.value; - } - } ); - return val; + const fileFields = document.querySelectorAll( + 'input[name="file' + fileID + '"], input[name="file' + fileID + '[]"], input[name^="item_meta[' + fileID + ']"]' + ); + + for ( const field of fileFields ) { + if ( field.value !== '' ) { + return field.value; + } + } + return ''; }Behaviour is unchanged (still effectively returns the first non-empty value) but avoids unnecessary iterations.
1096-1101:removeAddedScriptsstill depends on jQuery at call siteUsing
document.querySelectorAllwith aforEachto remove the end markers is a good jQuery reduction. Note thatformContainer.nextUntil(...).remove();still assumesformContaineris a jQuery object, which is true in the current call (replaceContent = jQuery( object ).closest( '.frm_forms' )). If you plan to keep refactoring jQuery out, this helper is a good candidate to fully convert to DOM traversal, or at least document thatformContainermust be jQuery.
1314-1328:removeSubmitLoadingnow affects all loading forms — confirm this is desiredThe new implementation ignores the first argument and operates on every
.frm_loading_formin the document, re-enabling submit/save-draft for all of them whenenable === 'enable'. That’s arguably simpler, but it’s a behavioural change if multiple forms can be in a loading state concurrently.If simultaneous AJAX submissions for multiple forms are possible, you may want to scope this to the specific form that initiated the call (using the first parameter) and only fall back to
querySelectorAllwhen no form is provided.If only one loading form is ever expected, this change is fine but it’s worth making that assumption explicit in a comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (4)
js/formidable.js (4)
317-341: SwitchingvalidateFieldto pass containers directly looks correctPassing
fieldContainer(anHTMLElementfromclosest) directly intoremoveFieldErrorandaddFieldErrormatches their signatures, which already accept either jQuery or DOM and normalize internally. This simplifies the call sites without changing behaviour.
1772-1776: NativequerySelectorAll+ jQuery blur trigger is consistentThe init logic that finds
input/textarea[onblur]viadocument.querySelectorAlland then callsjQuery( field ).trigger( 'blur' )for empty fields keeps the previous behaviour while reducing jQuery usage. No issues spotted here.
2044-2048: UsinggetComputedStylefor margins matches previous jQuery semanticsSwitching from jQuery’s
.css('margin-top')togetComputedStyle( document.documentElement/body ).marginTopkeeps the behaviour (string values like"0px"that are thenparseInt’d) while removing the jQuery dependency. Looks good.
1872-1887: Normalizeobjectto an HTMLElement before calling.closest
submitFormManualnow callsobject.closest( '.frmapi-form' ), which assumesobjectis anHTMLElement. Elsewhere in this module, form arguments commonly accept either jQuery or DOM (and normalize viainstanceof jQuery). If any external code callsfrmFrontForm.submitFormManual( e, jQuery( form ) ), this will throw aTypeError.You can make this robust while preserving current behaviour by normalizing once at the top:
submitFormManual: function( e, object ) { + const form = object instanceof jQuery ? object.get( 0 ) : object; + if ( ! form ) { + return; + } + - if ( document.body.classList.contains( 'wp-admin' ) && ! object.closest( '.frmapi-form' ) ) { + if ( document.body.classList.contains( 'wp-admin' ) && ! form.closest( '.frmapi-form' ) ) { return; }…and keep passing the original
objecttofrmProForm.submitAllowed,validateFormSubmit, andhasInvisibleRecaptcha, which already handle both jQuery and DOM.Since
submitFormManualis part of the publicfrmFrontFormAPI, this normalization guards against third‑party usage patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/formidable.js (1)
141-149: FixenableSubmitButtonJSDoc/param mismatch and accept jQuery|HTMLElement consistentlyThe JSDoc uses
$formwhile the function parameter isform, which is why ESLint reports both “expected @param names to beform” and “missing JSDoc @paramform”. Also, unlikedisableSubmitButton/enableSaveDraft, this now only works with a plain DOM element, which is a potential breaking change if anything still calls it with a jQuery object.You can align types, fix lint, and keep backward‑compatibility by normalizing like the other helpers:
/** - * Enable the submit button for a given jQuery form object + * Enable the submit button for a given form object. * * @since 2.03.02 * - * @param {HTMLElement} $form - * - * @return {void} - */ -function enableSubmitButton( form ) { - form.querySelectorAll( 'input[type="submit"], input[type="button"], button[type="submit"]' ).forEach( + * @param {jQuery|HTMLElement} form + * @return {void} + */ +function enableSubmitButton( form ) { + const formEl = form instanceof jQuery ? form.get( 0 ) : form; + if ( ! formEl ) { + return; + } + formEl.querySelectorAll( 'input[type="submit"], input[type="button"], button[type="submit"]' ).forEach( button => button.disabled = false ); }This should clear the jsdoc errors and keep existing jQuery call sites working.
♻️ Duplicate comments (1)
js/formidable.js (1)
1851-1859: Guard against missing form in reCAPTCHA callbacksIf the reCAPTCHA markup isn’t found or the element has been removed before the callback fires,
objectwill benull, andsubmitFormNow( object )will throw when it tries to accessobject.className.Adding a simple null‑check keeps normal flow intact and avoids hard failures in edge cases (same issue as previously noted in this file):
afterSingleRecaptcha: function() { - const recaptcha = document.querySelector( '.frm-show-form .g-recaptcha' ); - const object = recaptcha ? recaptcha.closest( 'form' ) : null; - frmFrontForm.submitFormNow( object ); + const recaptcha = document.querySelector( '.frm-show-form .g-recaptcha' ); + const object = recaptcha ? recaptcha.closest( 'form' ) : null; + if ( object ) { + frmFrontForm.submitFormNow( object ); + } }, afterRecaptcha: function( _, formID ) { - const object = document.querySelector( '#frm_form_' + formID + '_container form' ); - frmFrontForm.submitFormNow( object ); + const object = document.querySelector( '#frm_form_' + formID + '_container form' ); + if ( object ) { + frmFrontForm.submitFormNow( object ); + } },This prevents a null dereference if the form container can’t be found.
🧹 Nitpick comments (1)
js/formidable.js (1)
1095-1099: Consider scoping.frm_end_ajax_*lookup to the container for robustness
removeAddedScriptsnow queriesdocument.querySelectorAll( '.frm_end_ajax_' + formID ), then usesformContainer.nextUntil( '.frm_end_ajax_' + formID )from the (jQuery) container and removes all matched end markers globally.If the same
formIDcan appear multiple times on a page (e.g., duplicated form output), this will remove all matching end markers across the document, not just within this container’s context. If that’s not desired, consider scoping the selector:-function removeAddedScripts( formContainer, formID ) { - const endReplace = document.querySelectorAll( '.frm_end_ajax_' + formID ); +function removeAddedScripts( formContainer, formID ) { + const containerEl = formContainer instanceof jQuery ? formContainer.get( 0 ) : formContainer; + const endReplace = containerEl + ? containerEl.parentNode.querySelectorAll( '.frm_end_ajax_' + formID ) + : document.querySelectorAll( '.frm_end_ajax_' + formID );This keeps behavior local while still falling back to the global query if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(12 hunks)
🧰 Additional context used
🪛 GitHub Actions: Inspections
js/formidable.js
[error] 136-136: Missing JSDoc @param "form" declaration. (jsdoc/require-param)
🪛 GitHub Check: Run ESLint
js/formidable.js
[failure] 141-141:
Expected @param names to be "form". Got "$form"
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (9)
js/formidable.js (9)
169-183:enableSaveDraftDOM/jQuery handling looks solidNormalizing
$formto a DOM node and guarding against a missing form before iterating.frm_save_draftelements is correct and consistent withdisableSaveDraft. No issues here.
334-338: Passing rawfieldContainerinto error helpers is safe and simplifies thingsSwitching to pass the HTMLElement (
fieldContainer) directly intoremoveFieldError/addFieldErroris fine since both helpers already unwrap jQuery when needed. This reduces jQuery usage without changing behavior.
423-435:checkRequiredFieldvalue normalization preserves jQuery.val()semanticsThe new handling for
val === nulland non‑string values (arrays from multi‑selects) correctly normalizes to a single string, keeping the “empty vs non‑empty” logic intact. Behavior matches the prior jQuery.val()usage.
513-520:getFileValsjQuery→DOM refactor looks correctUsing
document.querySelectorAllover the three input name patterns and returning the first non‑emptyfield.valuemirrors the original jQuery.eachbehavior. Straightforward and safe change.
1317-1325: GlobalremoveSubmitLoadingbehavior is coherent with new usageIterating over all
.frm_loading_formelements, removing loading classes, firingfrmEndFormLoading, and (whenenable === 'enable') callingenableSubmitButton/enableSaveDraftper form aligns with howremoveSubmitLoadingis now called (sometimes with a specific form, sometimes with no arg fromenableSubmitButtonOnBackButtonPress). TheprocessesRunningguard is preserved. Looks good.
1677-1684: Back/forward cache handler correctly re‑enables submit buttonsUsing
document.querySelectorAll( '.frm_loading_form' )and callingenableSubmitButton( form )on each whenevent.persistedis true is consistent with the updatedenableSubmitButtonsignature, and the subsequentremoveSubmitLoading()safely clears loading state without re‑enabling drafts a second time. No issues.
1770-1773: Native selector + jQuery blur trigger is a clean hybridThe new init logic that uses
document.querySelectorAllto find.frm-show-forminputs/textarea withonblurand triggersjQuery( field ).trigger( 'blur' )only whenfield.value === ''preserves previous behavior while reducing reliance on jQuery for selection. Looks correct.
1871-1895: Admin‑page early return refactor is equivalentReplacing the jQuery check:
jQuery( 'body' ).hasClass( 'wp-admin' )! jQuery( object ).closest( '.frmapi-form' ).lengthwith:
if ( document.body.classList.contains( 'wp-admin' ) && ! object.closest( '.frmapi-form' ) ) { return; }preserves the original intent (skip front‑end submission handling for admin forms that aren’t
.frmapi-form) while avoiding jQuery selection. Safe and readable change.
2042-2046: UsinggetComputedStylefor margins inscrollMsgis fineSwitching from jQuery to:
m = getComputedStyle( document.documentElement ).marginTop; b = getComputedStyle( document.body ).marginTop;and subtracting their parsed integer values from
newPospreserves the previous offset‑adjustment behavior. Margin values are returned as CSS length strings (e.g.,"0px"), soparseIntis safe here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
js/formidable.js (1)
1847-1856: The previous review comment about null guards is still valid.The reCAPTCHA callbacks
afterSingleRecaptchaandafterRecaptchastill lack null guards before callingsubmitFormNow. If the DOM has changed or the form cannot be located,submitFormNow(null)will throw when accessingobject.classNameat line 1897.Apply the previously suggested fix:
afterSingleRecaptcha: function() { const recaptcha = document.querySelector( '.frm-show-form .g-recaptcha' ); const object = recaptcha ? recaptcha.closest( 'form' ) : null; + if ( object ) { frmFrontForm.submitFormNow( object ); + } }, afterRecaptcha: function( _, formID ) { const object = document.querySelector( '#frm_form_' + formID + '_container form' ); + if ( object ) { frmFrontForm.submitFormNow( object ); + } },
🧹 Nitpick comments (4)
js/formidable.js (4)
138-146: Consider accepting jQuery objects for backward compatibility.Unlike
enableSaveDraft(lines 172-176),enableSubmitButtonno longer handles jQuery objects. This is a breaking change for any external code calling this function with a jQuery-wrapped form.Apply this diff to match the pattern used in
enableSaveDraft:-function enableSubmitButton( form ) { +function enableSubmitButton( form ) { + form = form instanceof jQuery ? form.get( 0 ) : form; + if ( ! form ) { + return; + } form.querySelectorAll( 'input[type="submit"], input[type="button"], button[type="submit"]' ).forEach( button => button.disabled = false ); }
420-420: Replace jQuery.val()with native DOM.This line still uses jQuery for value retrieval, which is inconsistent with the PR's goal of replacing jQuery with native DOM APIs.
Apply this diff to use native DOM:
- val = jQuery( field ).val(); + if ( field.tagName === 'SELECT' && field.multiple ) { + val = Array.from( field.selectedOptions ).map( opt => opt.value ).filter( v => v !== '' ); + if ( val.length === 0 ) { + val = ''; + } else if ( val.length === 1 ) { + val = val[0]; + } + } else { + val = field.value || ''; + }
1091-1096: Refactor to eliminate jQuerynextUntil.This function receives a jQuery object and uses jQuery's
nextUntilmethod (line 1094), which is inconsistent with the PR's refactoring goal.Apply this diff to use native DOM:
function removeAddedScripts( formContainer, formID ) { + const container = formContainer instanceof jQuery ? formContainer.get( 0 ) : formContainer; + if ( ! container ) { + return; + } const endReplace = document.querySelectorAll( '.frm_end_ajax_' + formID ); if ( endReplace.length ) { - formContainer.nextUntil( '.frm_end_ajax_' + formID ).remove(); + let sibling = container.nextElementSibling; + while ( sibling && ! sibling.classList.contains( 'frm_end_ajax_' + formID ) ) { + const next = sibling.nextElementSibling; + sibling.remove(); + sibling = next; + } endReplace.forEach( el => el.remove() ); } }
1767-1771: Complete the jQuery replacement.While the field selection uses native DOM, the blur event is still triggered using jQuery. Consider using native APIs for consistency.
Apply this diff to use native event triggering:
document.querySelectorAll( '.frm-show-form input[onblur], .frm-show-form textarea[onblur]' ).forEach( function( field ) { if ( field.value === '' ) { - jQuery( field ).trigger( 'blur' ); + field.dispatchEvent( new Event( 'blur', { bubbles: true } ) ); } } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable.js(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
js/formidable.js (1)
stripe/js/frmstrp.js (3)
form(640-640)form(745-745)fieldContainer(941-941)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cypress
🔇 Additional comments (6)
js/formidable.js (6)
43-48: LGTM!The jQuery-to-native-element resolution is correctly implemented.
510-518: LGTM!The refactoring from jQuery to native DOM APIs is correctly implemented with proper iteration and value access.
331-331: LGTM!The calls to
removeFieldErrorandaddFieldErrorcorrectly pass native DOM elements, and both functions properly handle the conversion from jQuery or native at their entry points.Also applies to: 334-334
2039-2040: LGTM!The refactoring from jQuery's
.css()to nativegetComputedStyleis correctly implemented.
166-181: LGTM!The function properly handles both jQuery and native DOM elements with appropriate null safety and uses native DOM APIs for the implementation.
1868-1868: LGTM!The use of native
closest()method is correct.
Crabcyborg
changed the title
Try to replace more jQuery
1 participant
Related issue https://github.com/Strategy11/formidable-pro/issues/5002