New requirements for range sliders#2459
Conversation
WalkthroughRefactors field-insertion in js/formidable_admin.js: separates drag vs click post-response handling into two new public handlers, enriches insert args (adds section_id, last_row_field_ids, nonce), normalizes after-add hook name, and changes insertFormField Promise to resolve to the server message string. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant UI as Form Builder UI
participant B as FrmAdminBuild
participant S as Server
U->>UI: Drag field into form
UI->>B: insertNewFieldByDragging(placeholder, type, formId)
B->>S: AJAX insert (args include action, form_id, field_type, section_id, nonce, last_row_field_ids)
S-->>B: msg (insert response)
alt stopped by condition
B->>B: emit frm_stopped_inserting_by_dragging
else successful
B->>B: handleInsertFieldByDraggingResponse(msg, $placeholder)
B->>UI: insert/wrap/replace DOM, update order, set draggability
B->>UI: trigger frm_after_field_added_in_form_builder
end
sequenceDiagram
participant U as User
participant UI as Form Builder UI
participant B as FrmAdminBuild
participant S as Server
U->>UI: Click "Add field"
UI->>B: insertFormField(type, formId)
B->>S: AJAX insert (args include action, form_id, field_type, section_id, nonce, last_row_field_ids)
S-->>B: msg (insert response)
B->>B: handleAddFieldClickResponse(msg)
B->>UI: insert DOM, update layout, wire draggability
B->>UI: trigger frm_after_field_added_in_form_builder
B-->>UI: Resolve Promise with msg (server message string)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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: 3
🔭 Outside diff range comments (2)
js/formidable_admin.js (2)
1587-1595: Reintroduce an args filter for extensibility (and update @SInCE).Returning a plain object improves predictability, but it removes a useful extension point. To preserve BC and flexibility, apply a filter before returning. Also, replace the placeholder @SInCE x.x.
function getInsertNewFieldArgs( fieldType, sectionId, formId, hasBreak ) { - return { + const args = { action: 'frm_insert_field', form_id: formId, field_type: fieldType, section_id: sectionId, nonce: frmGlobal.nonce, has_break: hasBreak, last_row_field_ids: getFieldIdsInSubmitRow() - }; + }; + // Allow addons to adjust payload while keeping sensible defaults. + return wp.hooks.applyFilters( 'frm_insert_new_field_args', args, { fieldType, sectionId, formId, hasBreak } ); }Additionally, consider having getFieldIdsInSubmitRow filter out falsy dataset.fid values to avoid undefineds propagating.
2176-2201: Race risk: resolve before DOM insertion; afterAddField depends on the element being in DOM.insertFormField resolves immediately, leaving the caller to insert fieldElement. The subsequent setTimeout may run before insertion, causing afterAddField to fail (it queries the DOM by id). Make finalization wait until the element is connected, and align the hook payload/BC here too.
- fieldElement[0].style.display = 'none'; - resolve({ fieldElement, msg }); - - setTimeout( () => { - updateFieldOrder(); - afterAddField( msg, true ); - syncLayoutClasses( jQuery( fieldElement.closest( 'ul' ).children()[0] ) ); - fieldElement[0].style.display = 'block'; - - if ( fieldId ) { - wp.hooks.doAction( 'frm_after_field_added_in_form_builder', { - field: msg, - fieldId: fieldId, - fieldType: fieldType, - form_id: formId, - }); - } - }, 10 ); + fieldElement[0].style.display = 'none'; + resolve({ fieldElement, msg }); + + const finalize = () => { + // Wait until caller inserts the element into the DOM. + if ( ! fieldElement[0].isConnected ) { + return setTimeout( finalize, 16 ); + } + updateFieldOrder(); + afterAddField( msg, true ); + const $ul = jQuery( fieldElement.closest( 'ul' ) ); + if ( $ul.length ) { + syncLayoutClasses( jQuery( $ul.children()[0] ) ); + } + fieldElement[0].style.display = 'block'; + if ( fieldId ) { + const hookPayload = { + field: msg, + fieldId, + field_type: fieldType, + fieldType, + form_id: formId, + }; + wp.hooks.doAction( 'frm_after_field_added_in_form_builder', hookPayload ); + wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', hookPayload ); + } + }; + finalize();
🧹 Nitpick comments (1)
js/formidable_admin.js (1)
11185-11188: Public API: document new exports.The new handlers are exposed; ensure they’re documented in the public API (changelog + inline JSDoc) so plugin authors can safely rely on them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(7 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). (6)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
- 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)
1703-1709: No legacy event listeners or insertFormField consumers found
Grep across the codebase shows only the newfrm_after_field_added_in_form_builderhooks in js/formidable_admin.js (lines 1703, 2148, 2195) and the sole occurrence ofinsertFormField(is its definition. There are no downstream call sites or old‐name listeners to update.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
js/formidable_admin.js (3)
1653-1670: After-field-added hook: normalize payload and fire legacy hook for BC.The docblock lists snake_case keys, but the payload uses camelCase. Also, to avoid breaking existing integrations, fire the legacy event name alongside the new one.
Suggested change:
- wp.hooks.doAction( 'frm_after_field_added_in_form_builder', { - field: msg, - fieldId, - fieldType, - form_id: formId, - }); + const hookPayload = { + field: msg, + fieldId, // temporary: camelCase for transition + field_id: fieldId, // snake_case + fieldType, // temporary: camelCase for transition + field_type: fieldType, // snake_case per docs + form_id: formId, + }; + wp.hooks.doAction( 'frm_after_field_added_in_form_builder', hookPayload ); + // Back-compat + wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', hookPayload );Also replace “@SInCE x.x” in the nearby doc with the actual version.
2109-2130: Mirror payload normalization and legacy hook firing in click handler.Match the drag path’s payload keys and fire the legacy hook to preserve integrations.
- wp.hooks.doAction( 'frm_after_field_added_in_form_builder', { - field: msg, - fieldId, - fieldType, - form_id: formId, - }); + const hookPayload = { + field: msg, + fieldId, + field_id: fieldId, + fieldType, + field_type: fieldType, + form_id: formId, + }; + wp.hooks.doAction( 'frm_after_field_added_in_form_builder', hookPayload ); + wp.hooks.doAction( 'frmadmin.afterFieldAddedInFormBuilder', hookPayload );
1615-1619: Emit structured payload and add click-path parity event for stop-insert.Pass a structured object to frm_stopped_inserting_by_dragging so the payload is future‑proof and include a parity event for clicks.
Apply within this hunk:
- if ( shouldStopInsertingField( fieldType ) ) { - wp.hooks.doAction( 'frm_stopped_inserting_by_dragging', fieldType ); + if ( shouldStopInsertingField( fieldType ) ) { + wp.hooks.doAction( 'frm_stopped_inserting_by_dragging', { field_type: fieldType, source: 'drag' } ); return; }And mirror in addFieldClick (outside this diff) so consumers can react consistently:
// Inside addFieldClick(), before the early return: if ( shouldStopInsertingField( fieldType ) ) { wp.hooks.doAction( 'frm_stopped_inserting_by_clicking', { field_type: fieldType, source: 'click' } ); return; }
🧹 Nitpick comments (1)
js/formidable_admin.js (1)
1587-1595: Centralized insert args look good; minor doc nit.Returning a plain object here is clean and explicit. Please also update the surrounding JSDoc to replace “@SInCE x.x” with the real version before release.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
js/formidable_admin.js (1)
js/formidable.js (1)
fieldId(1976-1976)
⏰ 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 (4)
js/formidable_admin.js (4)
1677-1710: Refactor extraction LGTM.Good separation of concerns in handleInsertFieldByDraggingResponse; the flow (wrap/replace, order sync, layout sync, droppable/draggable wiring) is clear and matches existing patterns.
2136-2155: Click-response handler extraction LGTM.Solid symmetry with the drag handler, including wrapping, positioning relative to submit, and wiring droppable/draggable.
11180-11184: Public API expansion: confirm intended exports.Exporting the two response handlers matches the PR goal. Confirm that exposing confirmLinkClick and syncLayoutClasses is intentional (they weren’t mentioned in the objectives) to avoid accidental surface area growth.
2172-2172: insertFormField now resolves a string; verify call sites and update JSDocI ran a grep across the repo for both
insertFormField(and any.then(...).append/replaceWith/etc.usage and only found the definition injs/formidable_admin.js(around line 2157). No consumers treating the result as a DOM/jQuery element were detected, and there isn’t a JSDoc block for this function.Please:
- Add or update the JSDoc for
insertFormFieldinjs/formidable_admin.js(≈ line 2157) to document that it now resolves to an HTML string.- Manually verify any external or asynchronous callers (including
await insertFormField(...)) across the codebase or integrations to ensure none expect a DOM/jQuery object.
1 participant
frm_insert_field_argsis no longer necessaryfrm_stopped_inserting_by_draggingused for tracking the drag placeholder.frmadmin.afterFieldAddedInFormBuilderhook. We're trying to use a more consistent name scheme to our PHP filters, so it has been changed tofrm_after_field_added_in_form_builderinstead.insertFormFieldnow returnsmsg. I removed code to hide and show fields that I feel should be handled in the code that callsinsertFormFieldinstead of inside of the function as it complicated things.syncLayoutClassesis now also global. This is used in the date ranges PR.