add filter that will extended the form field container html attributes…#2428
Conversation
…, add js filter that will get fired after a field is successfully added in the form build, export 2 js functions that are related to inserting a new field and deleting it from form builder in order to reuse these in other plugins, toggle a jQuery notice if jQuery is selected as library from general settings
WalkthroughAdds optional field_options to new-field creation in PHP and JavaScript, introduces a filter for extra field container attributes, updates the view to render those attributes, adds a default range_field setting, enhances admin JS with an insertFormField API and related hooks, and adjusts datepicker loading to be conditional on Pro capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant AdminUI as Admin UI (Form Builder)
participant JS as formidable_admin.js
participant Server as FrmFieldsController
participant View as add_field.php
AdminUI->>JS: frmAdminBuildJS.insertFormField(type, options)
JS->>Server: POST insert field (type, form_id, field_options)
Server->>Server: include_new_field(type, form_id, field_options)
Server->>View: Render field HTML (apply extra attributes filter)
Server-->>JS: HTML response (data-fid=ID)
JS->>JS: Parse fieldId, insert HTML, update order/UI
JS-->>AdminUI: Resolve Promise with element
JS->>AdminUI: fire frmadmin.afterFieldAddedInFormBuilder({field, fieldId, fieldType, form_id})
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
classes/controllers/FrmFieldsController.php (1)
191-200: Update the version placeholder in PHPDoc.The filter implementation looks good, but the version placeholder should be updated to reflect the actual version.
- * @since x.x + * @since 6.13Regarding the static analysis hint about unused variable: this appears to be a false positive since the variable is used in the view file as mentioned in the summary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
classes/controllers/FrmFieldsController.php(4 hunks)classes/controllers/FrmStylesController.php(1 hunks)classes/models/fields/FrmFieldType.php(1 hunks)classes/views/frm-fields/back-end/settings.php(1 hunks)classes/views/frm-forms/add_field.php(1 hunks)css/admin/frm_admin_global.css(1 hunks)js/admin/settings.js(2 hunks)js/admin/style.js(1 hunks)js/formidable_admin.js(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
js/formidable_admin.js (1)
js/admin/legacy-views.js (1)
thisFormId(2-2)
🪛 GitHub Check: Psalm
classes/controllers/FrmStylesController.php
[failure] 543-543: InvalidScalarArgument
classes/controllers/FrmStylesController.php:543:76: InvalidScalarArgument: Argument 2 of is_callable expects bool, but 'use_jquery_datepicker' provided (see https://psalm.dev/012)
🪛 GitHub Check: PHPStan
classes/controllers/FrmStylesController.php
[failure] 543-543:
Call to an undefined static method FrmProAppHelper::use_jquery_datepicker().
🪛 GitHub Actions: Psalm Code Analysis
classes/controllers/FrmStylesController.php
[error] 543-543: Psalm: InvalidScalarArgument: Argument 2 of is_callable expects bool, but 'use_jquery_datepicker' provided (see https://psalm.dev/012)
🪛 PHPMD (2.15.0)
classes/controllers/FrmFieldsController.php
200-200: Avoid unused local variables such as '$extra_field_attributes'. (Unused Code Rules)
(UnusedLocalVariable)
🔇 Additional comments (11)
classes/views/frm-forms/add_field.php (1)
6-6: Variable Definition VerifiedI confirmed that
$extra_field_attributesis defined inclasses/controllers/FrmFieldsController.phpvia theapply_filters( 'frm_field_container_extra_attributes', … )call and is in scope when the templateclasses/views/frm-forms/add_field.phpis included. The use ofesc_attr()in the template appropriately sanitizes the output. No further action is needed.classes/models/fields/FrmFieldType.php (1)
370-370: range_field setting integrated consistentlyThe
range_fielddefault was added todefault_field_settings()inclasses/models/fields/FrmFieldType.php, and your search shows it’s properly checked and defaulted inclasses/controllers/FrmFieldsController.php:
- In FrmFieldsController.php, you guard against an undefined index:
if ( ! isset( $field['range_field'] ) ) { $field['range_field'] = false; }No other references to update. This setting is now fully supported with a safe default and appropriate checks.
js/admin/settings.js (2)
17-19: LGTM!The event handler integration follows the existing pattern and is properly implemented.
55-71: Well-implemented function with proper documentation.The function has good JSDoc documentation, clear logic, and proper DOM manipulation. The conditional check for 'jquery' library value and appropriate CSS class toggling are well executed.
js/admin/style.js (1)
1449-1453: Excellent conflict prevention implementation.The conditional check properly prevents jQuery UI datepicker initialization when Pro's flatpickr is available, avoiding potential conflicts. The explanatory comment clearly documents the purpose.
js/formidable_admin.js (1)
11064-11065: LGTM! Function exports enable reusability.The export of
deleteFieldandinsertFormFieldfunctions enables their reuse in other plugins as intended by the PR objectives.classes/controllers/FrmFieldsController.php (5)
67-69: LGTM! Proper parameter handling with appropriate sanitization.The new
field_optionsparameter is correctly retrieved and sanitized usingwp_kses_post, which is appropriate for HTML content that may contain field options.
73-73: LGTM! Method call updated consistently.The method call correctly passes the new
field_optionsparameter to match the updated method signature.
86-86: LGTM! Method signature and documentation updated correctly.The optional
$field_optionsparameter with default empty array maintains backward compatibility while enabling the new functionality.Also applies to: 90-90
93-95: LGTM! Clean merge logic with proper validation.The implementation correctly:
- Checks if field_options is not empty before merging
- Uses
array_mergeto combine options, allowing field_options to override defaults- Maintains the existing structure
335-337: LGTM! Good defensive programming practice.Adding the
range_fieldsafeguard prevents undefined index errors and ensures consistent field structure across all field types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
stubs.php (1)
82-83: Add documentation and return type annotation for better IDE support.The new
use_jquery_datepicker()method should include proper documentation and return type annotation to improve code clarity and IDE support.Apply this diff to add proper documentation and return type:
+ /** + * @return bool + */ public static function use_jquery_datepicker() { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/models/fields/FrmFieldType.php(1 hunks)classes/views/frm-fields/back-end/settings.php(1 hunks)js/formidable_admin.js(5 hunks)stubs.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- classes/views/frm-fields/back-end/settings.php
- classes/models/fields/FrmFieldType.php
- js/formidable_admin.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2428 +/- ##
=========================================
Coverage 27.47% 27.48%
- Complexity 8702 8706 +4
=========================================
Files 140 140
Lines 28733 28739 +6
=========================================
+ Hits 7895 7898 +3
- Misses 20838 20841 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
js/formidable_admin.js (6)
1680-1697: DRY: factor the repeated hook firing into a helper functionSame hook firing logic appears in 3 places. Create a small helper and reuse it.
Example helper (place near other utilities):
function fireAfterFieldAddedHook( msg, fieldType, formId ) { const m = msg.match(/data-fid="(\d+)"/); if (!m) { return; } wp.hooks.doAction('frmadmin.afterFieldAddedInFormBuilder', { field: msg, field_id: m[1], field_type: fieldType, form_id: formId, }); }Then here:
- if ( fieldId ) { - /** - * Fires after a field is added. - * - * @since x.x - * - * @param {Object} fieldData The field data. - * @param {String} fieldData.field The field HTML. - * @param {String} fieldData.field_type The field type. - * @param {String} fieldData.form_id The form ID. - */ - wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { - field: msg, - fieldId: fieldId, - fieldType: fieldType, - form_id: formId, - }); - } + fireAfterFieldAddedHook( msg, fieldType, formId );
2108-2125: Unify hook payload keys with docs (snake_case), and reuse helperThe payload uses camelCase and duplicates logic again. Align keys and call a helper.
Minimal key rename if not extracting a helper:
- wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { - field: msg, - fieldId: fieldId, - fieldType: fieldType, - form_id: formId, - }); + wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { + field: msg, + field_id: fieldId, + field_type: fieldType, + form_id: formId, + });Or replace the whole block with:
- if ( fieldId ) { - ... - } + fireAfterFieldAddedHook( msg, fieldType, formId );
2178-2184: Unify hook payload keys; or replace with helperKeep payload consistent with JSDoc and other calls. Better: call the shared helper.
- wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { - field: msg, - fieldId: fieldId, - fieldType: fieldType, - form_id: formId, - }); + wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { + field: msg, + field_id: fieldId, + field_type: fieldType, + form_id: formId, + });Or:
- // fire hook here - ... + fireAfterFieldAddedHook( msg, fieldType, formId );
2090-2090: Repeat: guard regex match to prevent runtime errorSame issue as above;
msg.match(...)[1]can be null. Apply the same null-check.- const fieldId = msg.match( /data-fid="(\d+)"/ )[1]; + const fieldIdMatch = msg.match( /data-fid="(\d+)"/ ); + const fieldId = fieldIdMatch ? fieldIdMatch[1] : null;
2157-2158: Repeat: guard regex match to prevent runtime errorSame null-check issue for
fieldIdin insertFormField.- const fieldId = msg.match( /data-fid="(\d+)"/ )[1]; + const fieldIdMatch = msg.match( /data-fid="(\d+)"/ ); + const fieldId = fieldIdMatch ? fieldIdMatch[1] : null;
1653-1654: Guard against null regex matches when extracting fieldId
msg.match(/data-fid="(\d+)"/)[1]will throw if the pattern isn’t present. Store the match result and check it before indexing.Apply this diff:
- const fieldId = msg.match( /data-fid="(\d+)"/ )[1]; + const fieldIdMatch = msg.match( /data-fid="(\d+)"/ ); + const fieldId = fieldIdMatch ? fieldIdMatch[1] : null;
🧹 Nitpick comments (3)
js/formidable_admin.js (3)
1680-1697: Align hook payload keys with docs and WP conventions; JSDoc uses snake_caseThe JSDoc block documents
field_typebut the payload usesfieldType. Also consider usingfield_idrather thanfieldIdfor consistency. Update both keys and the JSDoc if needed.- wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { - field: msg, - fieldId: fieldId, - fieldType: fieldType, - form_id: formId, - }); + wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { + field: msg, + field_id: fieldId, + field_type: fieldType, + form_id: formId, + });
2146-2154: Use getInsertNewFieldArgs to avoid duplicated request data (per teammate’s note)Leverage the existing helper, then add
field_options.- data: { - action: 'frm_insert_field', - form_id: formId, - field_type: fieldType, - field_options: fieldOptions, - section_id: 0, - nonce: frmGlobal.nonce, - has_break: hasBreak, - last_row_field_ids: getFieldIdsInSubmitRow() - }, + data: ( () => { + const data = getInsertNewFieldArgs( fieldType, 0, formId, hasBreak ); + data.field_options = fieldOptions; + return data; + } )(),
2132-2132: Add a brief comment for the setTimeout rationale (why 10ms?)If a delay is still needed after resolving, add a one-line comment explaining it (e.g. “allow layout reflow before updating classes”).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(5 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). (3)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (1)
js/formidable_admin.js (1)
11165-11168: Exports look good and align with PR goalExposing
showSaveAndReloadModal,deleteField, andinsertFormFieldmatches the PR’s extensibility objectives.
…irmMoldal on link href attribute search
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
js/formidable_admin.js (1)
344-347: Fix undefined variable in confirm modal; href/data-href fallback looks good
- Critical: for (i in dataAtts) runs before dataAtts is assigned. This will throw at runtime. Loop over removeAtts instead.
- The new href fallback to data-href on the continue button is a sensible enhancement. LGTM.
Apply this diff:
- removeAtts = continueButton.dataset; - for ( i in dataAtts ) { - continueButton.removeAttribute( 'data-' + i ); - } + removeAtts = continueButton.dataset; + for ( i in removeAtts ) { + continueButton.removeAttribute( 'data-' + i ); + }Also applies to: 367-367
♻️ Duplicate comments (5)
classes/views/frm-forms/add_field.php (1)
6-6: Good fix: render extra attributes via array_to_html_paramsThis resolves the earlier escaping/quote issues and outputs well-escaped key="value" pairs.
js/formidable_admin.js (4)
1653-1653: Guard regex when extracting fieldId (can throw if no match)msg.match(/data-fid="(\d+)"/)[1] will throw if the pattern isn’t found. Guard the match before indexing.
Apply this diff:
- const fieldId = msg.match( /data-fid="(\d+)"/ )[1]; + const fieldIdMatch = msg.match( /data-fid="(\d+)"/ ); + const fieldId = fieldIdMatch ? fieldIdMatch[1] : null;
1680-1697: DRY: Refactor duplicated hook firing into a helperThe logic to fire frmadmin.afterFieldAddedInFormBuilder is repeated 3x. Extract it to a single helper to reduce duplication and future maintenance risk. Also benefits centralizing the safe fieldId extraction.
Add once (near other helpers):
function fireAfterFieldAddedHook( msg, fieldType, formId ) { const m = msg.match( /data-fid="(\d+)"/ ); if ( ! m ) { return; } wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { field: msg, fieldId: m[1], fieldType, form_id: formId, } ); }Then replace the repeated blocks with:
- if ( fieldId ) { - /** - * Fires after a field is added. - * - * @since x.x - * ... - */ - wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { - field: msg, - fieldId: fieldId, - fieldType: fieldType, - form_id: formId, - }); - } + fireAfterFieldAddedHook( msg, fieldType, formId );Also applies to: 2108-2125, 2169-2175
2091-2091: Guard regex when extracting fieldId in addFieldClickSame issue: msg may not contain data-fid, leading to a crash when indexing [1].
Apply this diff:
- const fieldId = msg.match( /data-fid="(\d+)"/ )[1]; + const fieldIdMatch = msg.match( /data-fid="(\d+)"/ ); + const fieldId = fieldIdMatch ? fieldIdMatch[1] : null;
2132-2181: insertFormField has critical flaws: no DOM insertion, unsafe regex, no Promise reject
- DOM: afterAddField, updateFieldOrder, and syncLayoutClasses assume the field exists in the builder DOM. This function never appends msg to the DOM, so these calls are unsafe.
- Regex: unsafe [1] indexing can throw.
- Promise: no reject path on AJAX errors.
Rewrite to mirror addFieldClick flow (append to DOM, init DnD, then fire hooks), add a reject path, and guard regex.
Apply this diff:
- function insertFormField( fieldType, fieldOptions = {} ) { - - return new Promise( ( resolve ) => { + function insertFormField( fieldType, fieldOptions = {} ) { + return new Promise( ( resolve, reject ) => { const formId = thisFormId; let hasBreak = 0; if ( 'summary' === fieldType ) { hasBreak = $newFields.children( 'li[data-type="break"]' ).length > 0 ? 1 : 0; } jQuery.ajax({ type: 'POST', url: ajaxurl, - data: Object.assign( getInsertNewFieldArgs( fieldType, 0, formId, hasBreak ), { field_options: fieldOptions } ), - success: function( msg ) { - const fieldElement = jQuery( msg ); - const fieldId = msg.match( /data-fid="(\d+)"/ )[1]; - - fieldElement[0].style.display = 'none'; - resolve( fieldElement ); - setTimeout( () => { - updateFieldOrder(); - afterAddField( msg, true ); - syncLayoutClasses( jQuery( fieldElement.closest( 'ul' ).children()[0] ) ); - fieldElement[0].style.display = 'block'; - - if ( fieldId ) { - /** - * Fires after a field is added. - * - * @since x.x - * - * @param {Object} fieldData The field data. - * @param {String} fieldData.field The field HTML. - * @param {String} fieldData.field_type The field type. - * @param {String} fieldData.form_id The form ID. - */ - wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { - field: msg, - fieldId: fieldId, - fieldType: fieldType, - form_id: formId, - }); - } - }, 10 ); - }, - error: handleInsertFieldError + data: ( () => { + const args = getInsertNewFieldArgs( fieldType, 0, formId, hasBreak ); + args.field_options = fieldOptions; + return args; + } )(), + success: function( msg ) { + document.getElementById( 'frm_form_editor_container' ).classList.add( 'frm-has-fields' ); + const replaceWith = wrapFieldLi( msg ); + const submitField = $newFields[0].querySelector( '.edit_field_type_submit' ); + if ( submitField ) { + jQuery( submitField.closest( '.frm_field_box:not(.form-field)' ) ).before( replaceWith ); + } else { + $newFields.append( replaceWith ); + } + // Init DnD on inserted field(s) + replaceWith.each( function() { + makeDroppable( this.querySelector( 'ul.frm_sorting' ) ); + makeDraggable( this.querySelector( '.form-field' ), '.frm-move' ); + } ); + updateFieldOrder(); + afterAddField( msg, true ); + // Fire after-added hook safely + const m = msg.match( /data-fid="(\d+)"/ ); + if ( m ) { + wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { + field: msg, + fieldId: m[1], + fieldType: fieldType, + form_id: formId, + } ); + } + resolve( replaceWith ); + }, + error: function( xhr, status, error ) { + handleInsertFieldError( xhr, status, error ); + reject( error || status || 'Insert field failed' ); + } }); } ); }Optional follow-ups:
- Consider honoring shouldStopInsertingField(fieldType) the same way addFieldClick does.
- Document what the Promise resolves to (the wrapper li.frm_field_box) and update JSDoc.
🧹 Nitpick comments (6)
classes/views/frm-forms/add_field.php (1)
6-6: Optional: restrict attribute keys for stricter safetyIf you want to prevent on* event attributes for users without unfiltered_html, consider filtering keys server-side (eg, allow data-, aria-, role, id, class, style) before rendering. This keeps the API flexible while reducing risk.
classes/controllers/FrmFieldsController.php (3)
86-90: Doc nit: clarify $field_options and add @SInCE for new parameterPlease expand the param description and add an @SInCE tag for the new argument to aid integrators.
Suggested doc tweak:
- Describe $field_options as “Optional. Keys to override defaults in field_options at creation.”
- Add “@SInCE $field_options argument added.”
93-96: Confirm merge semantics; consider recursive merge for nested optionsarray_merge is shallow; nested arrays under field_options will be fully replaced. If you want to preserve nested defaults while overriding only provided leaves, use array_replace_recursive.
- $field_values['field_options'] = array_merge( $field_values['field_options'], $field_options ); + $field_values['field_options'] = array_replace_recursive( $field_values['field_options'], $field_options );If full replacement is intentional, ignore this suggestion.
191-201: Filter docs and safety: finalize @SInCE and optionally screen unsafe keys
- Replace @SInCE x.x with the actual release.
- Optional: For users without unfiltered_html, drop unsafe attribute names (eg on*). This mirrors your existing input attribute gating and reduces accidental XSS vectors in admin.
- $extra_field_attributes = apply_filters( 'frm_field_container_extra_attributes', array(), $field, $display ); + $extra_field_attributes = apply_filters( 'frm_field_container_extra_attributes', array(), $field, $display ); + // For users who cannot post unfiltered HTML, restrict unsafe attribute names. + if ( FrmAppHelper::should_never_allow_unfiltered_html() ) { + $extra_field_attributes = array_filter( + (array) $extra_field_attributes, + function ( $v, $k ) { + return FrmAppHelper::input_key_is_safe( $k ); + }, + ARRAY_FILTER_USE_BOTH + ); + }PHPMD “Unused local variable” is a false positive (used in included template). You can suppress it in the method docblock:
/** * @param array|int|object $field_object * @param array $values * @param int $form_id * @SuppressWarnings(PHPMD.UnusedLocalVariable) $extra_field_attributes is used in included template (add_field.php). */js/formidable_admin.js (2)
1684-1690: Replace @SInCE x.x with the actual version before mergeThese hook docs still show @SInCE x.x. Please set to the release version for traceability.
Also applies to: 2112-2118, 2162-2168
11156-11159: New public exports — looks good; add changelog/API notesExporting showSaveAndReloadModal, deleteField, insertFormField, and confirmLinkClick improves extensibility. LGTM. Please:
- Add these to the public API docs/changelog with @SInCE version.
- Note insertFormField’s resolved value type in JSDoc.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/controllers/FrmFieldsController.php(3 hunks)classes/views/frm-fields/back-end/settings.php(1 hunks)classes/views/frm-forms/add_field.php(1 hunks)js/formidable_admin.js(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/views/frm-fields/back-end/settings.php
🧰 Additional context used
🧬 Code Graph Analysis (3)
classes/controllers/FrmFieldsController.php (2)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4602)get_post_param(682-692)classes/helpers/FrmFieldsHelper.php (1)
FrmFieldsHelper(6-2425)
classes/views/frm-forms/add_field.php (1)
classes/helpers/FrmAppHelper.php (2)
FrmAppHelper(6-4602)array_to_html_params(1366-1375)
js/formidable_admin.js (2)
js/admin/legacy-views.js (4)
link(274-274)msg(98-98)formId(231-231)thisFormId(2-2)js/formidable.js (1)
fieldId(1976-1976)
🪛 PHPMD (2.15.0)
classes/controllers/FrmFieldsController.php
200-200: Avoid unused local variables such as '$extra_field_attributes'. (Unused Code Rules)
(UnusedLocalVariable)
⏰ 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). (3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/controllers/FrmFieldsController.php (1)
73-74: LGTM: pass field_options into include_new_fieldKeeps the create flow consistent with the new API.
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
Thanks @Liviu-p!
I removed the jQuery logic from here, and removed a CSS class that I don't think we need.
I think we can merge this now.
🚀
Crabcyborg
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
js/formidable_admin.js (3)
2121-2138: DRY the hook firing (duplicate of previous suggestion)Same duplication as earlier. Please reuse a helper as suggested above.
1680-1697: DRY the repeated hook firing into a helperThis block is duplicated in multiple places. Extract a helper and reuse to avoid drift and improve readability.
+function fireAfterFieldAddedHook( msg, fieldType, formId ) { + const fieldId = checkMsgForFieldId( msg ); + if ( fieldId ) { + wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { + field: msg, + fieldId, + // Keep naming consistent (see note below). + fieldType: fieldType, + form_id: formId, + }); + } +}Then replace this block and the other occurrences with:
- if ( fieldId ) { - /** - * Fires after a field is added. - * @since x.x - * @param {Object} fieldData The field data. - * @param {String} fieldData.field The field HTML. - * @param {String} fieldData.field_type The field type. - * @param {String} fieldData.form_id The form ID. - */ - wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { - field: msg, - fieldId: fieldId, - fieldType: fieldType, - form_id: formId, - }); - } + fireAfterFieldAddedHook( msg, fieldType, formId );Note: the documented key is field_type but the emitted key is fieldType. Pick one and keep it consistent across docs and payload. See separate comment.
2145-2194: Critical: insertFormField never inserts into the DOM but calls DOM-dependent codeThe function resolves with a detached jQuery element and then calls updateFieldOrder(), afterAddField(), and syncLayoutClasses() which assume the field exists in the builder DOM. This can throw (e.g., afterAddField queries document.getElementById on ids that don't exist yet).
Fix by inserting the returned HTML into the builder (consistent with addFieldClick/drag flow), initializing DnD, then updating state and firing the hook. Also, reject on AJAX error.
-function insertFormField( fieldType, fieldOptions = {} ) { - - return new Promise( ( resolve ) => { +function insertFormField( fieldType, fieldOptions = {} ) { + return new Promise( ( resolve, reject ) => { const formId = thisFormId; let hasBreak = 0; if ( 'summary' === fieldType ) { hasBreak = $newFields.children( 'li[data-type="break"]' ).length > 0 ? 1 : 0; } jQuery.ajax({ type: 'POST', url: ajaxurl, - data: Object.assign( getInsertNewFieldArgs( fieldType, 0, formId, hasBreak ), { field_options: fieldOptions } ), + data: Object.assign( getInsertNewFieldArgs( fieldType, 0, formId, hasBreak ), { field_options: fieldOptions } ), success: function( msg ) { - const fieldElement = jQuery( msg ); - const fieldId = checkMsgForFieldId( msg ); - - fieldElement[0].style.display = 'none'; - resolve( fieldElement ); - setTimeout( () => { - updateFieldOrder(); - afterAddField( msg, true ); - syncLayoutClasses( jQuery( fieldElement.closest( 'ul' ).children()[0] ) ); - fieldElement[0].style.display = 'block'; - - if ( fieldId ) { - /** - * Fires after a field is added. - * - * @since x.x - * - * @param {Object} fieldData The field data. - * @param {String} fieldData.field The field HTML. - * @param {String} fieldData.field_type The field type. - * @param {String} fieldData.form_id The form ID. - */ - wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { - field: msg, - fieldId: fieldId, - fieldType: fieldType, - form_id: formId, - }); - } - }, 10 ); + document.getElementById( 'frm_form_editor_container' ).classList.add( 'frm-has-fields' ); + + // Build wrapper consistent with other flows and insert before submit row when present. + const $wrapper = wrapFieldLi( msg ); + const submitField = $newFields[0].querySelector( '.edit_field_type_submit' ); + if ( submitField ) { + jQuery( submitField.closest( '.frm_field_box:not(.form-field)' ) ).before( $wrapper ); + } else { + $newFields.append( $wrapper ); + } + + // Initialize DnD on the inserted node(s). + $wrapper.each( function() { + makeDroppable( this.querySelector( 'ul.frm_sorting' ) ); + makeDraggable( this.querySelector( '.form-field' ), '.frm-move' ); + }); + + // Update builder state and fire events. + updateFieldOrder(); + afterAddField( msg, true ); + // Fire JS hook in a safe, DRY helper. + if ( typeof fireAfterFieldAddedHook === 'function' ) { + fireAfterFieldAddedHook( msg, fieldType, formId ); + } else { + const fieldId = checkMsgForFieldId( msg ); + if ( fieldId ) { + wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', { + field: msg, + fieldId, + fieldType, + form_id: formId, + }); + } + } + + resolve( $wrapper ); }, - error: handleInsertFieldError + error: function( xhr, status, error ) { + handleInsertFieldError( xhr, status, error ); + reject( error || status || 'Insert field failed' ); + } }); - } ); + }); }Optional: remove the setTimeout; after insertion, DOM is ready synchronously.
🧹 Nitpick comments (3)
js/formidable_admin.js (3)
367-367: Avoid setting href to "null" when neither href nor data-href is presentIf both attributes are missing, setAttribute will convert null to the string "null". Guard before setting or remove the attribute.
-continueButton.setAttribute( 'href', link.getAttribute( 'href' ) || link.getAttribute( 'data-href' ) ); +const href = link.getAttribute( 'href' ) || link.getAttribute( 'data-href' ); +if ( href ) { + continueButton.setAttribute( 'href', href ); +} else { + continueButton.removeAttribute( 'href' ); +}
1704-1715: Helper LGTM; minor nit: specify radix in parseIntThe function is solid. Add radix 10 for parseInt.
- return result ? parseInt( result[1] ) : 0; + return result ? parseInt( result[1], 10 ) : 0;
1684-1691: Align docs with payload and set correct @SInCE
- The documented property uses field_type but payload uses fieldType. Align either the docs or the payload to a single name.
- Replace "@SInCE x.x" with the actual version before merge.
Also applies to: 2126-2133, 2176-2183
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
js/admin/settings.js(0 hunks)js/formidable_admin.js(6 hunks)
💤 Files with no reviewable changes (1)
- js/admin/settings.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
js/formidable_admin.js (2)
js/admin/legacy-views.js (4)
link(274-274)msg(98-98)formId(231-231)thisFormId(2-2)js/formidable.js (1)
fieldId(1976-1976)
🔇 Additional comments (3)
js/formidable_admin.js (3)
1654-1654: Good: safe extraction of fieldIdSwitching to checkMsgForFieldId prevents regex null-dereference errors. LGTM.
2104-2104: Good: safe fieldId extractionUsing checkMsgForFieldId here avoids match-null errors. LGTM.
11169-11173: Public API surface change — confirm intent and stabilityAdding showSaveAndReloadModal, deleteField, insertFormField, confirmLinkClick to the exported object widens the public API. Confirm:
- Which are intended to be stable for third-party use (docs needed)?
- Whether exporting deleteField is desired (it performs destructive actions), or should it remain internal.
2 participants
…, add js filter that will get fired after a field is successfully added in the form build, export 2 js functions that are related to inserting a new field and deleting it from form builder in order to reuse these in other plugins, toggle a jQuery notice if jQuery is selected as library from general settings
This PR is a requirement for https://github.com/Strategy11/formidable-pro/pull/5902 and https://github.com/Strategy11/formidable-dates/pull/168.