Duplicate unsaved field opts #1488
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1488 +/- ##
============================================
+ Coverage 29.60% 29.62% +0.01%
+ Complexity 7860 7811 -49
============================================
Files 121 119 -2
Lines 25952 25705 -247
============================================
- Hits 7684 7615 -69
+ Misses 18268 18090 -178 ☔ View full report in Codecov by Sentry. |
AbdiTolesa
changed the title
Crabcyborg
requested review from
garretlaxton
and removed request for
garretlaxton
WalkthroughThe changes enhance the functionality of the form builder interface in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- js/formidable_admin.js (1 hunks)
Additional comments: 4
js/formidable_admin.js (4)
- 2146-2146: The addition of the
maybeDuplicateUnsavedOptionsfunction call within the field duplication process is a crucial step towards ensuring that field options are accurately duplicated. This aligns well with the PR's objective of enhancing the form builder's functionality.- 2181-2216: The
copyUnsavedDeleteOptionsfunction is designed to sync the options editing elements. It iterates over the options of the original field and duplicates them for the new field. This function is crucial for maintaining the integrity of field options during duplication.One potential improvement is in the check for
originalOpts. ThequerySelectorAllmethod returns a NodeList, which is truthy even when empty. The checkif (!originalOpts)will never be true, making it redundant.Remove the redundant check for
originalOptsasquerySelectorAllalways returns a NodeList, which is truthy even when it contains no nodes.
- 2218-2239: The
copyUnsavedOptionsfunction effectively duplicates the options in the form preview area. It ensures that the visual representation of the form matches the actual options available. This is an essential part of the user experience in the form builder.This function appears to be well-implemented, with appropriate checks for the existence of options containers before proceeding. The loop to duplicate each option is straightforward and effective.
- 2241-2252: The
replaceElementAttributefunction plays a critical role in ensuring that references to field IDs and keys are updated correctly in the duplicated options. The use of regular expressions to replace these references is a smart approach, allowing for a clean and efficient update process.However, there's a potential improvement in the creation of the temporary div (
tempDiv). Each call toreplaceElementAttributecreates a new div element, which might be inefficient if duplicating a large number of options.Consider optimizing the creation of temporary elements when duplicating a large number of options. One approach could be reusing a single temporary div across calls to
replaceElementAttributeif possible, to reduce the overhead of DOM element creation.
| /** | ||
| * Duplicates field options that are not saved after being modified to the new field. | ||
| * | ||
| * @param {Number} originalFieldId | ||
| * @param {string} newFieldHtml | ||
| * @returns {void} | ||
| */ | ||
| function maybeDuplicateUnsavedOptions( originalFieldId, newFieldHtml ) { | ||
| const newFieldId = jQuery( newFieldHtml ).attr( 'data-fid' ); | ||
| const newOptsContainer = document.getElementById( `frm_field_${newFieldId}_opts` ); | ||
| const origOptsContainer = document.getElementById( `frm_field_${originalFieldId}_opts` ); | ||
|
|
||
| if ( ! newOptsContainer || ! origOptsContainer || ! newOptsContainer.dataset.key || ! origOptsContainer.dataset.key ) { | ||
| return; | ||
| } | ||
| newOptsContainer.innerHTML = ''; | ||
|
|
||
| const newFieldKey = newOptsContainer.dataset.key; | ||
| const originalFieldKey = origOptsContainer.dataset.key; | ||
|
|
||
| const args = { originalFieldId, originalFieldKey, newFieldId, newFieldKey }; | ||
|
|
||
| copyUnsavedDeleteOptions( origOptsContainer, newOptsContainer, args ); | ||
|
|
||
| copyUnsavedOptions( args ); | ||
| } |
There was a problem hiding this comment.
The maybeDuplicateUnsavedOptions function is well-documented and its purpose is clear. It checks for the presence of options containers for both the original and new fields before proceeding with the duplication of options. This is a good practice as it ensures that the function operates only when necessary, potentially avoiding errors or unintended behavior.
However, there's an opportunity to improve error handling and logging. In scenarios where the options containers are not found or the data keys are missing, silently returning might make debugging difficult if the duplication does not work as expected.
Consider adding console warnings or logging to help identify when the function exits early due to missing containers or data keys. This could be invaluable for debugging purposes.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
js/formidable_admin.js (3)
Line range hint
1-2275: Overall structure looks good, but review methods furtherThe
frmAdminBuildJSfunction returns a large object with many methods that initialize the UI. While the overall structure seems okay, the individual methods on the returned object should be analyzed further to ensure they are implemented correctly and follow best practices.Consider breaking out some of the larger methods into separate named functions to improve readability and maintainability.
Line range hint
2326-2522:buildInitworks but could use refactoringThe
buildInitmethod sets up the form builder interface and wires up all the needed event listeners. The individual code sections look like they are working properly. However, this is a very long method that is doing a lot.Consider breaking this up into smaller methods grouped by responsibility to improve readability and maintainability. The helper functions called should also be reviewed separately.
Line range hint
2524-2693:settingsInitmethod looks goodThe
settingsInitmethod properly handles initializing the form settings page and setting up the required event listeners for the settings. Functionally it looks good.Consider refactoring some of the larger event listener functions into separate named functions to clean up this method a bit and make it more readable.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- js/formidable_admin.js (1 hunks)
Additional comments not posted (2)
js/formidable_admin.js (2)
Line range hint
2277-2324:initmethod looks goodThe
initmethod handles high-level initialization of the UI by calling more specific init methods and setting up some global event listeners. This looks fine. The other init methods that are called should be reviewed separately.
Line range hint
2695-2729:panelInitmethod looks goodThe
panelInitmethod properly handles initializing a custom panel and setting up the required event listeners. The code looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (8)
js/formidable_admin.js (8)
Line range hint
1-2275: Overall Structure Looks Good, But Consider Breaking Up Large FunctionsThe
frmAdminBuildJSfunction follows a modular pattern by returning an object with many function properties. This is a good architectural approach for organizing related functionality.However, the function is quite large, with many nested functions. Consider refactoring some of the larger nested functions into separate, smaller functions if possible. This would improve readability and maintainability.
That said, the overall structure and organization is solid. Nice work encapsulating everything into an IIFE to avoid polluting the global scope.
Line range hint
2276-2474: init Function Structure Is SolidThe
initfunction is well-organized and broken into logical sections for initializing different parts of the admin functionality. The comments help explain what each section is responsible for.The code looks solid overall, but a couple minor suggestions:
- Consider using
addEventListenerinstead ofjQuery.onfor attaching event listeners. It's more performant and aligns with native browser APIs. For example:document.addEventListener('event', handler); // instead of jQuery(document).on('event', handler);
- If this function gets much larger over time, consider refactoring some of the sections into separate initialization functions that
initcan call. This would keepinitmore concise and focused.But overall, the
initfunction is in good shape and follows a clear organizational structure. Good job!
Line range hint
2476-2692: buildInit Is Well-Organized But Watch For Performance On Many Event ListenersThe
buildInitfunction follows a similar structure toinit, with clear organization and separation of responsibilities. The comments are helpful for understanding what each block of code is doing.One potential concern is the performance impact of attaching a large number of event listeners, especially if there are many elements on the page that match the selectors.
If this becomes a problem, consider using event delegation instead of attaching listeners to individual elements. With event delegation, you attach the listener to a parent element and check the event target to conditionally handle the event. For example:
jQuery('#parent-element').on('click', '.child-selector', handler);This attaches one event listener to the parent element instead of many listeners to individual child elements.
But overall, the
buildInitfunction looks good and is easy to follow. Just keep an eye on performance and refactor to use event delegation if needed.
Line range hint
2694-2701: maybeAddFieldSelection Looks Good With Minor Selector OptimizationThis function is clear and focused on adding a class to certain elements if they don't already have it.
One small optimization would be to avoid using
hasClassin the selector and instead use a single selector for better performance:var missingClass = jQuery(`${parentClass} .frm_not_email_message:not(.frm_has_shortcodes), ${parentClass} .frm_not_email_to:not(.frm_has_shortcodes), ${parentClass} .frm_not_email_subject:not(.frm_has_shortcodes)`);This avoids an extra function call to
hasClassfor each element. But in most cases, the performance difference will likely be negligible.Other than that, the function looks good! The code is clear and easy to understand.
Line range hint
2703-2715: getFieldValues Works But Needs Error HandlingThis function retrieves field values via an AJAX request, which is a common pattern in the admin script.
However, it's missing error handling for the AJAX request. If the request fails for some reason (network issue, server error, etc.), the code assumes success and tries to update the DOM with the response.
To make this more robust, add a
.fail()callback to handle errors gracefully:jQuery.ajax({ // ... success: function(msg) { // ... }, fail: function(jqXHR, textStatus, errorThrown) { console.error('Error retrieving field values:', textStatus, errorThrown); // Display user-friendly error message } });This will prevent errors if the AJAX request doesn't succeed.
Also, consider using single quotes consistently for strings to match the style used in most of the file.
But overall, the AJAX functionality itself looks good. Just add that error handling to make it bulletproof.
Line range hint
2717-2722: showElement Is Simple and SolidThis function is short, sweet, and to the point. It shows an element by setting its display style to an empty string.
Checking if the element exists in the DOM first is a nice defensive programming touch to avoid errors when calling this function.
The only thing to keep in mind is the dependency on jQuery. If the codebase ever moves away from jQuery, this function would need a small refactoring to use native browser methods instead:
function showElement(element) { if (element && element.style) { element.style.display = ''; } }But that's a minor future-proofing nitpick. For now, the function is simple, clear, and does exactly what it needs to do. Nice work!
Line range hint
2724-2747: markRequired Works Well But Could Be SimplifiedThis function properly toggles the required state of a field when the required checkbox is clicked. It handles various elements and classes to show/hide the required styling and settings.
The logic all looks good and covers the necessary parts of the UI that need to change based on the checkbox state.
A couple suggestions for improvement:
Consider breaking out some of the functionality into smaller helper functions. For example, toggling the visibility of the required indicator and updating its value could be its own function. This would make the main function more readable at a glance.
The function uses a mix of vanilla JS methods (getElementById, this.checked) and jQuery (closest, find). For consistency and readability, consider sticking with one or the other. If you do use jQuery, you could chain some of the calls to simplify the code:
var $reqBox = jQuery(this).closest('.frm_required_details').find('input[name="field_options[required_indicator_' + fieldId + ']"]');But overall, the functionality is solid and the required state is properly synced with the UI. The suggestions are just for code clarity and consistency. Nice job on handling all the necessary UI updates!
Line range hint
2749-2766: markUnique Toggles Setting WellThis function nicely toggles the visibility of the unique field setting based on the checkbox state.
It finds the relevant elements and shows/hides them appropriately, including the parent container if the unique setting is the only visible row.
The jQuery flows logically and is used consistently for a concise implementation.
One suggestion for improvement would be to extract the duplicated code for finding visible rows into a helper function:
function countVisibleRows($parent) { return $parent.find('.frm_validation_box:visible').length; }Then the main function could be simplified to:
if (this.checked) { $thisField.fadeIn('slow').closest('.frm_validation_msg').fadeIn('slow'); // ... } else { var visibleRows = countVisibleRows($thisField.closest('.frm_validation_box')); if (visibleRows === 0) { $thisField.closest('.frm_validation_msg').fadeOut('slow'); } }This would keep the main function focused on the unique setting logic and avoid duplicating the row counting code.
But overall, the function does a great job of toggling the unique field setting as needed. The jQuery is used effectively to find and manipulate the relevant elements. Well done!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable_admin.js (1)
2201-2236: Consider error handling and logging.The
copyUnsavedDeleteOptionsfunction syncs the options editing elements used to add, modify or delete options in the field options area. It looks functionally correct.However, there's an opportunity to improve error handling and logging. In scenarios where the options containers are not found or the data keys are missing, silently returning might make debugging difficult if the duplication does not work as expected.
Consider adding console warnings or logging to help identify when the function exits early due to missing containers or data keys. This could be invaluable for debugging purposes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- js/formidable_admin.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
js/formidable_admin.js (4)
2174-2199: Looks good!The
maybeDuplicateUnsavedOptionsfunction is well-documented and its purpose is clear. It checks for the presence of options containers for both the original and new fields before proceeding with the duplication of options. This is a good practice as it ensures that the function operates only when necessary, potentially avoiding errors or unintended behavior.
2238-2259: Looks good!The
copyUnsavedOptionsfunction syncs the options in the form preview area. It retrieves the original and new field option containers, clears the new container, and copies over the child elements from the original to the new container.The function has clear comments, uses meaningful variable names, and the logic flow is easy to follow. It handles the case when the original or new containers are not found by returning early.
2261-2274: Looks good!The
replaceElementAttributefunction replaces the original field ID or key with the new one in the provided element's HTML string. It uses a temporarydivelement to parse the modified HTML string and returns the first child node.The function is well-structured, uses clear variable names, and has a straightforward logic flow. It handles replacing field IDs and keys in various patterns using the
replaceFieldIdPatternsfunction.
2276-2292: Looks good!The
replaceFieldIdPatternsfunction replaces the original field ID or key with the new one in the provided element string. It constructs a regular expression pattern based on the providedstartCharandendCharparameters to match and replace the field ID or key.The function uses a clear naming convention for the parameters, making it easy to understand their purpose. The regular expression is constructed dynamically based on the input parameters, allowing flexibility in matching different patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable_admin.js (1)
2196-2196: Consider passingorigOptsContainerandnewOptsContaineras arguments tocopyUnsavedOptions.Since
copyUnsavedOptionsis called withargswhich already contains the field IDs and keys, it may be cleaner to also pass the options containers as part ofargs. This would keep all the required data consolidated in a single object.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- js/formidable_admin.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
js/formidable_admin.js (5)
2174-2199: Looks good!The
maybeDuplicateUnsavedOptionsfunction is well-documented and its purpose is clear. It checks for the presence of options containers for both the original and new fields before proceeding with the duplication of options. This is a good practice as it ensures that the function operates only when necessary, potentially avoiding errors or unintended behavior.
2201-2236: ThecopyUnsavedDeleteOptionsfunction looks good!It iterates over the original options, replaces necessary attributes using
replaceElementAttribute, and copies over the option values and keys. The comments clearly explain the purpose of the function.
2238-2259: ThecopyUnsavedOptionsfunction is implemented correctly.It retrieves the original and new field options containers, clears the new container, and iterates over the original options to create new options with replaced attributes. The comments provide a clear explanation of what the function does.
2261-2274: ThereplaceElementAttributefunction looks good.It takes an element and
argsobject, replaces the field ID and key patterns in the element's HTML string, and returns a new element with the replaced attributes. The function is well-documented with comments explaining its purpose.
2276-2292: ThereplaceFieldIdPatternsfunction is implemented correctly.It replaces the original field ID/key prepended by
startCharand followed byendCharwith the new field ID/key in the givenelementString. The function uses a regular expression to perform the replacement and returns the updated string.
Fix https://github.com/Strategy11/formidable-pro/issues/3726
This PR extends to making sure field options are synced when a field is duplicated for Checkbox, Radio and Dropdown fields.
Test steps