Prevent duplicating field from breaking field group limit#2209
Conversation
WalkthroughThe changes introduce a new function, Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant DF as duplicateField
participant CDF as canDuplicateField
participant M as Modal Display
U->>DF: Request field duplication
DF->>CDF: Check if duplication allowed
alt Allowed
CDF-->>DF: Return eligible
DF->>U: Perform duplication
else Not Allowed
CDF-->>DF: Return not eligible
DF->>M: Trigger information modal
M-->>U: Display warning message
end
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@AbdiTolesa I tested this and it's working as expected. Thank you!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/formidable_admin.js (1)
2104-2124: Implementation looks good but could use better documentationThe function correctly prevents duplicating fields in collapsed pages and enforces the max field group limit. Consider adding JSDoc documentation for better code maintainability.
Add JSDoc documentation like:
/** * Returns true if a field can be duplicated. * * @since x.x * @param {HTMLElement} field The field element to check * @param {number} maxFieldsInGroup Maximum number of fields allowed in a group * @returns {Boolean} Whether the field can be duplicated */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (3)
js/formidable_admin.js (3)
2126-2203: LGTM! Field duplication validation is properly integratedThe changes correctly prevent field duplication when validation fails and show a user-friendly translated error message. The existing duplication logic is preserved when validation passes.
10107-10148: LGTM! Icon handling is well implementedThe function properly adds save and drag icons to field options while avoiding duplicates. It handles both new and existing options consistently.
10150-10164: LGTM! Field options icon handling is well implementedThe function correctly adds icons to all relevant field options while properly excluding the first option and "other" options.
truongwp
left a comment
truongwp
left a comment
There was a problem hiding this comment.
This looks good. Thanks @AbdiTolesa!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
js/formidable_admin.js (2)
2104-2124: New function to validate field duplication looks good but could use JSDoc improvementsThe new
canDuplicateFieldfunction correctly validates whether a field can be duplicated based on field group limits. However, the JSDoc comments could be improved by:
- Adding
@returnstag to document the boolean return value- Adding more details about the
maxFieldsInGroupparameter/** * Returns true if a field can be duplicated. * * @since x.x * + * @param {HTMLElement} field The field element to check + * @param {number} maxFieldsInGroup Maximum number of fields allowed in a group + * @returns {Boolean} True if the field can be duplicated, false otherwise */
2126-2203: Field duplication integration looks good but could use error message improvementThe integration of
canDuplicateFieldin theduplicateFieldfunction is well implemented. However, the error message could be improved to be more specific.- infoModal( sprintf( __( 'You can only have a maximum of %1$d fields in a field group. Delete or move out a field from the group and try again.', 'formidable' ), maxFieldsInGroup ) ); + infoModal( sprintf( __( 'This field group already has the maximum of %1$d fields. Please remove a field or create a new group to add more fields.', 'formidable' ), maxFieldsInGroup ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cypress
🔇 Additional comments (1)
js/formidable_admin.js (1)
1943-1953: Field group limit validation logic is solidThe
groupCanFitAnotherFieldfunction properly validates field group limits by:
- Allowing up to 6 fields per group
- Handling the edge case where a field is being moved within the same group
…breaking_group_limit
|
@AbdiTolesa It looks like there might be some sort of bug with the duplicating. Right now it's trying to show the group on multiple lines if I create a new group, then duplicate a field. I think it's happening on master too, but we should fix that while we're working on this since it's so related. Screen.Recording.2025-02-10.at.12.51.26.PM.mov |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
js/formidable_admin.js (2)
2104-2124: Add JSDoc documentation for better code clarity.The implementation looks good but would benefit from JSDoc documentation to clearly describe the parameters and return value.
/** * Returns true if a field can be duplicated. * * @since x.x * @param {HTMLElement} field The field element to check * @param {number} maxFieldsInGroup Maximum number of fields allowed in a group * @returns {boolean} Whether the field can be duplicated */ function canDuplicateField( field, maxFieldsInGroup ) { - if ( field.classList.contains( 'frm-page-collapsed' ) ) { + // Prevent duplicating fields in collapsed pages + if ( field.classList.contains( 'frm-page-collapsed' ) ) { return false; } - const fieldGroup = field.closest( 'li.frm_field_box:not(.form-field)' ); + const fieldGroupContainer = field.closest( 'li.frm_field_box:not(.form-field)' ); - if ( ! fieldGroup ) { + if ( ! fieldGroupContainer ) { return true; } - const fieldsInGroup = getFieldsInRow( jQuery( fieldGroup.querySelector( 'ul' ) ) ).length; + const fieldsInGroup = getFieldsInRow( jQuery( fieldGroupContainer.querySelector( 'ul' ) ) ).length; return fieldsInGroup < maxFieldsInGroup; }
2133-2135: Consider improving error message maintainability.While the error message implementation works well, consider extracting the message template to a constant or configuration object for easier maintenance and reuse.
+ const FIELD_GROUP_MESSAGES = { + MAX_FIELDS: __( 'You can only have a maximum of %1$d fields in a field group. Delete or move out a field from the group and try again.', 'formidable' ) + }; - infoModal( sprintf( __( 'You can only have a maximum of %1$d fields in a field group. Delete or move out a field from the group and try again.', 'formidable' ), maxFieldsInGroup ) ); + infoModal( sprintf( FIELD_GROUP_MESSAGES.MAX_FIELDS, maxFieldsInGroup ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cypress
🔇 Additional comments (1)
js/formidable_admin.js (1)
2132-2136: LGTM! Validation logic is properly integrated.The validation check is well integrated into the duplicateField function with a clear error message and proper internationalization.
…p_limit' of https://github.com/Strategy11/formidable-forms into issue-5223-prevent_duplicating_field_from_breaking_group_limit
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
classes/controllers/FrmFieldsController.php (1)
203-207: LGTM! The class handling logic is improved.The changes enhance the robustness of class assignment by:
- Using
!empty()instead ofisset()to ensure classes are only appended when they contain a value- Adding a fallback to use default classes when none are specified
Consider extracting the default class retrieval to a separate method for better maintainability:
- $li_classes .= FrmAppHelper::get_param( 'field_classes', '', 'post', 'sanitize_text_field' ) . ' '; + $li_classes .= self::get_default_field_classes() . ' ';And add the new method:
/** * Get default field classes when none are specified. * * @return string */ private static function get_default_field_classes() { return FrmAppHelper::get_param( 'field_classes', '', 'post', 'sanitize_text_field' ) . ' '; }js/formidable_admin.js (1)
2185-2219: Field class synchronization preserves layout after duplicationThe code properly synchronizes CSS classes when duplicating fields within field groups.
Consider extracting the class synchronization logic into a separate reusable function to improve maintainability. For example:
function syncDuplicatedFieldClasses( replaceWith, hoverTarget, fieldId ) { if ( !hoverTarget || !isFieldGroup( hoverTarget.parentElement ) ) { return; } const fieldClasses = document.getElementById( 'frm_classes_' + fieldId ).value; if ( !replaceWith.get(0).className.includes( fieldClasses ) ) { replaceWith.get(0).className += ' ' + fieldClasses; document.getElementById( 'frm_classes_' + replaceWith.get(0).dataset.fid ).value = fieldClasses; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/controllers/FrmFieldsController.php(1 hunks)js/formidable_admin.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cypress
🔇 Additional comments (3)
classes/controllers/FrmFieldsController.php (1)
203-207: Verify the relationship with field duplication fix.While these changes improve class handling, could you clarify how they relate to preventing field duplication from breaking field group limits? The main fix appears to be in
js/formidable_admin.jswith the newcanDuplicateFieldfunction.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for field duplication related code rg -A 5 "canDuplicateField|duplicateField" js/Length of output: 2937
Clarification on how CSS class handling supports the duplication fix
The changes in
FrmFieldsController.phpimprove how CSS classes are assigned to fields—ensuring that fields render with clean, expected class names. This consistency is important because the JavaScript duplication logic (specifically thecanDuplicateFieldfunction injs/formidable_admin.js) relies on proper class markers (e.g. detectingfrm-page-collapsedor locating the correct field group viaclosest()). In short, the PHP update is a supporting change to guarantee that duplicated fields have the correct classes for UI and group limit validation, while the actual limit enforcement remains in the JavaScript fix.js/formidable_admin.js (2)
2104-2124: New function enforces field group size limitThe new
canDuplicateFieldfunction properly validates if a field can be duplicated by checking:
- If the field is not in a collapsed page
- If adding another field would exceed the maximum allowed fields in a group (6)
The implementation follows best practices by:
- Using clear parameter names
- Including JSDoc documentation
- Properly handling edge cases when fieldGroup is not found
2132-2136: Field duplication validation prevents exceeding group limitsThe validation is properly integrated into the duplicateField function, showing an informative error message when a field cannot be duplicated.
The error message uses sprintf() for proper string formatting and translation support.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
js/formidable_admin.js (3)
2104-2124: Field duplication validation looks good but consider adding JSDoc param typesThe new
canDuplicateFieldfunction provides good validation logic for field duplication. The implementation correctly checks for collapsed pages and field group limits.Consider adding JSDoc param type definitions for better code documentation and IDE support:
/** * Returns true if a field can be duplicated. * * @since x.x + * @param {HTMLElement} field The field element to check + * @param {number} maxFieldsInGroup Maximum number of fields allowed in a group * @returns {Boolean} */
2132-2136: Good validation message implementation but consider i18nThe error message for field group limit is properly implemented using
sprintf()and__()for translation.Consider moving the error message to a reusable constant or configuration object to keep all messages in one place for easier maintenance.
2216-2231: Layout class handling looks good but add error handlingThe
setLayoutClassesForFieldInGroupfunction effectively copies layout classes between fields.Consider adding:
- Error handling for cases where elements are not found
- Validation for the fieldId parameter
- Documentation for the expected HTML structure
📜 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 (1)
- GitHub Check: Cypress
🔇 Additional comments (1)
js/formidable_admin.js (1)
2180-2180: Good layout class synchronization after field duplicationThe code properly syncs layout classes after field duplication in both single field and field group contexts.
The implementation ensures layout consistency is maintained when duplicating fields.
Also applies to: 2202-2202
@Crabcyborg I looked into it and found out that the layout classes for the field that is duplicated were not saved in the database and PHP is not aware of that when it copies the field. That's why the copy field's layout is not in sync but that is fixed now. |
…breaking_group_limit
|
Thank you @AbdiTolesa! This is looking great. I just found one more issue though. It looks like I cannot duplicate a group of fields with 6 fields in it. I made a short recording. You can see how I can duplicate a group with 5 fields just fine, and it creates a new group with 5 fields. This 6 field check is preventing me from duplicating a group that has 6 fields, which should work fine. I should just have 2 groups with 6 fields in each. Screen.Recording.2025-02-13.at.12.49.49.PM.mov |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
js/formidable_admin.js (5)
2104-2124: Field duplication validation looks good but could use a constantThe validation logic for checking if a field can be duplicated is well implemented. However, the maximum fields limit of 6 should be defined as a constant at the top of the file rather than hardcoding the number in the function.
// At the top of the file with other constants + const MAX_FIELDS_IN_GROUP = 6; // In the canDuplicateField function - return fieldsInGroup < maxFieldsInGroup; + return fieldsInGroup < MAX_FIELDS_IN_GROUP;This would make the code more maintainable if the limit needs to be changed in the future.
2132-2136: Consider enhancing the error message UXThe error message implementation for field duplication is good but could be enhanced:
- The message could be more specific about which field group is at the limit
- Consider adding a visual indicator on the field group that's at its limit
- infoModal( sprintf( __( 'You can only have a maximum of %1$d fields in a field group. Delete or move out a field from the group and try again.', 'formidable' ), maxFieldsInGroup ) ); + const groupLabel = fieldGroup.querySelector('.frm-group-label')?.textContent || __( 'this group', 'formidable' ); + infoModal( sprintf( + __( 'You can only have a maximum of %1$d fields in %2$s. Delete or move out a field from the group and try again.', 'formidable' ), + maxFieldsInGroup, + groupLabel + ) ); + fieldGroup.classList.add('frm-group-at-limit');
2215-2230: Add validation and error handling to layout class syncThe layout class synchronization for duplicated fields is well implemented, but could benefit from additional validation and error handling:
function setLayoutClassesForDuplicatedFieldInGroup( field, newField ) { + if ( ! field || ! newField ) { + console.warn( 'Missing required fields for layout class sync' ); + return; + } const hoverTarget = field.closest( '.frm-field-group-hover-target' ); if ( ! hoverTarget || ! isFieldGroup( hoverTarget.parentElement ) ) { return; } const fieldId = field.dataset.fid; + if ( ! fieldId ) { + console.warn( 'Field ID is required for layout class sync' ); + return; + } let fieldClasses = document.getElementById( 'frm_classes_' + fieldId )?.value; if ( ! fieldClasses ) { return; } fieldClasses = fieldClasses.replace( 'frm_first', '' ); if ( ! newField.className.includes( fieldClasses ) ) { newField.className += ' ' + fieldClasses; + try { document.getElementById( 'frm_classes_' + newField.dataset.fid ).value = fieldClasses; + } catch ( error ) { + console.error( 'Error syncing layout classes:', error ); + } } }
2132-2136: Consider using template literals for message formattingWhile sprintf works well for i18n, consider using template literals for better readability when formatting messages:
- infoModal( sprintf( __( 'You can only have a maximum of %1$d fields in a field group. Delete or move out a field from the group and try again.', 'formidable' ), maxFieldsInGroup ) ); + const message = __( 'You can only have a maximum of ${maxFieldsInGroup} fields in a field group. Delete or move out a field from the group and try again.', 'formidable' ); + infoModal( message.replace( '${maxFieldsInGroup}', maxFieldsInGroup ) );This would make the code more readable while maintaining i18n compatibility.
2104-2124: Add JSDoc documentation for better maintainabilityThe functions would benefit from more detailed JSDoc documentation:
+/** + * Validates if a field can be duplicated based on page state and group limits. + * + * @param {HTMLElement} field - The field element to validate + * @param {number} maxFieldsInGroup - Maximum number of fields allowed in a group + * @returns {boolean} True if the field can be duplicated + * @throws {Error} If field element is invalid + */ function canDuplicateField( field, maxFieldsInGroup ) { // ... existing code }This would improve code maintainability and IDE support.
…p_limit' of https://github.com/Strategy11/formidable-forms into issue-5223-prevent_duplicating_field_from_breaking_group_limit
Thank you for catching that earlier and this is fixed now. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/formidable_admin.js (1)
2104-2124: LGTM! New validation function prevents breaking field group limits.The new
canDuplicateField()function properly validates that fields can only be duplicated when:
- The field is not in a collapsed page
- The field's group has not reached the maximum limit
Consider defining a constant for the max fields per group limit.
The maximum of 6 fields per group is currently hardcoded. Consider defining this as a constant at the top of the file for better maintainability.
const MAX_FIELDS_PER_GROUP = 6;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
js/formidable_admin.js
[error] 2133-2133: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
js/formidable_admin.js (2)
2215-2230: LGTM! Layout classes are properly synchronized for duplicated fields.The new
setLayoutClassesForDuplicatedFieldInGroup()function ensures layout consistency by:
- Copying layout classes from the original field to the duplicated field
- Properly handling the 'frm_first' class to maintain correct ordering
- Checking for proper field group context before applying changes
2133-2137: LGTM! Field duplication now properly validates group limits.The
duplicateField()function has been enhanced to:
- Validate field group limits before duplication
- Show a clear error message when the limit would be exceeded
- Maintain layout consistency after successful duplication
🧰 Tools
🪛 Biome (1.9.4)
[error] 2133-2133: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
Thanks @AbdiTolesa @truongwp and @lauramekaj1 for working on this!
I'm seeing some other issues with sections, but it's nothing new. I'll log another issue shortly for that.
For now, I'm going to merge this.
🚀
4 participants
Fix https://github.com/Strategy11/formidable-pro/issues/5223
Screen record
Test steps