Show/Hide some shortcodes contextually#2052
Conversation
WalkthroughThis pull request introduces enhancements to the management of contextual shortcodes within the form builder. Key changes include the addition of new functions in Changes
Possibly related PRs
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable_admin.js (1)
Line range hint
8450-8598: Refactor suggestion: Extract contextual shortcode logic into separate functions.The new code for adding and removing contextual shortcodes based on the input field is quite lengthy and makes the
showShortcodeBoxfunction harder to read. Consider extracting the logic for getting contextual shortcodes and adding/removing them into separate helper functions. This will improve code organization and readability.For example, you could have separate functions like:
function getContextualShortcodes() { ... } function addContextualShortcodes(input) { ... } function removeContextualShortcodes() { ... }And then call them from
showShortcodeBoxas needed. This will make the main function more focused and easier to understand at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- js/formidable_admin.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
js/formidable_admin.js (1)
8600-8600: LGTM!The addition of the
removeContextualShortcodescall in thehideShortcodesfunction looks good. It ensures that any contextual shortcodes are properly removed when hiding the shortcode box.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
classes/views/shared/mb_adv_info.php (1)
213-216: LGTM! Consider using an array for improved readability.The implementation correctly hides the 'admin_email' and 'default-from-email' shortcodes, aligning with the PR objective. This change enhances the user interface by concealing potentially sensitive or infrequently used shortcodes.
For improved readability, consider using an array to store the shortcodes to be hidden:
$hidden_shortcodes = array( 'admin_email', 'default-from-email' ); if ( in_array( $skey, $hidden_shortcodes, true ) ) { $classes .= ' frm_hidden'; }This approach would make it easier to add or remove shortcodes from the hidden list in the future.
js/formidable_admin.js (5)
Line range hint
1-8450: Approve overall structure, but consider modularizationThe
frmAdminBuildJSfunction implements a module pattern, which is a good practice for encapsulation and organizing code. It sets up various components and event listeners, providing a public API through the returned object. However, given its size and complexity, consider breaking it down into smaller, more focused modules to improve maintainability and readability.Consider splitting this large function into smaller, more focused modules. This could improve code organization, make it easier to maintain, and potentially improve performance by allowing for more granular loading of functionality.
Line range hint
108-420: Comprehensive form builder initialization, but consider further modularizationThe
buildInitfunction provides a thorough setup for the form builder interface, covering various aspects of functionality. It effectively sets up event listeners and initializes different components of the builder. The consistent use of jQuery for event binding is noted.While the function is well-structured, its length and the variety of concerns it handles suggest that it could benefit from further modularization. Consider breaking it down into smaller, more focused functions, each responsible for initializing a specific aspect of the form builder. This could improve readability and make the code easier to maintain and test.
Line range hint
759-761: Properly marked deprecated functionThe
customCSSInitfunction is correctly marked as deprecated with a console warning. This is a good practice for phasing out old functionality while maintaining backwards compatibility.Consider adding a timeline for complete removal of this function in future versions. Also, if there's a replacement function or method, it would be helpful to mention it in the deprecation warning.
Line range hint
1012-1191: Consider refactoring global functions for better encapsulationThis section contains several global functions that handle various UI interactions and form operations. While they provide necessary functionality, their global nature could potentially lead to naming conflicts and make the code harder to maintain.
Consider refactoring these global functions into the main
frmAdminBuildJSmodule or separate modules. This would improve encapsulation, reduce the risk of naming conflicts, and make the code more maintainable. Additionally, updating the coding practices to use more modern approaches (like usingconst/letinstead ofvar, and leveraging more ES6+ features) could improve code quality and readability.
Line range hint
1193-1240: Effective initialization with Bootstrap compatibility handlingThe jQuery document ready function provides a good entry point for initializing the admin build functionality when the DOM is fully loaded. The inclusion of Bootstrap dropdown setup and conversion to Bootstrap 4 demonstrates attention to compatibility and user interface consistency.
Consider using the newer
DOMContentLoadedevent instead of jQuery's document ready for better performance and to reduce dependency on jQuery. Also, if possible, move the Bootstrap conversion logic into a separate function for better code organization.classes/controllers/FrmFormsController.php (2)
Line range hint
4-4: Reminder: Address the TODO comment.The TODO comment indicates that tests are missing for this function. Please ensure that the additional parameter change is thoroughly tested to confirm that it behaves as expected.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
Line range hint
12-24: Consider adjusting the fee structure or discount policy.The implementation of a flat $20 fee on discounted bills could negate the benefit of the discount, especially for smaller purchases or marginal loyalty tiers. This might lead to customer dissatisfaction, as the intent to reward loyalty paradoxically increases the bill.
Consider revising either the discount percentages or the flat fee application to better align with customer incentives.
Would you like assistance in generating a revised implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- classes/controllers/FrmFormsController.php (1 hunks)
- classes/views/shared/mb_adv_info.php (1 hunks)
- js/formidable_admin.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (16)
js/formidable_admin.js (15)
Line range hint
9-106: Well-structured initialization with conditional component loadingThe
initfunction is well-organized and serves as the main entry point for the admin build functionality. It effectively uses conditional checks to initialize different components based on the presence of specific DOM elements. This approach is commendable as it allows for efficient, on-demand initialization of features, potentially improving performance and modularity.The conditional initialization based on DOM elements is a good practice, ensuring that only relevant code is executed for the current page or context.
Line range hint
422-529: Well-organized settings initialization with good event handlingThe
settingsInitfunction is well-structured and focused on initializing settings-related functionality. It effectively sets up event listeners for various settings actions, including form actions, email settings, and other configuration options. The use of event delegation is a good practice for efficiency.The function's organization and focus on settings-specific functionality make it clear and maintainable. The moderate size and clear purpose of this function are commendable.
Line range hint
531-638: Focused panel initialization with effective event handlingThe
panelInitfunction is well-organized and focused on initializing panel-related functionality. It efficiently sets up event listeners for various panel actions, including shortcode insertion, logic building, and input focus events. The use of event delegation is a good practice for performance.The function's focused approach to panel-specific functionality and its moderate size contribute to code clarity and maintainability. The organization of different panel-related initializations within a single function is logical and effective.
Line range hint
640-684: Well-structured view initialization with effective event handlingThe
viewInitfunction is well-organized and focused on initializing view-related functionality. It efficiently sets up event listeners for various view actions, including content tabs, shortcodes, and form selection. The use of jQuery for DOM manipulation and event handling is consistent and effective.The function's focused approach to view-specific functionality and its moderate size contribute to code clarity and maintainability. The organization of different view-related initializations within a single function is logical and effective.
Line range hint
686-743: Efficient inbox initialization with effective AJAX handlingThe
inboxInitfunction is well-structured and focused on initializing inbox-related functionality. It efficiently sets up event listeners for dismissing inbox messages and handles AJAX requests for message dismissal. The use of AJAX for asynchronous updates is a good practice for improving user experience.The function's focused approach to inbox-specific functionality and its efficient handling of message dismissal contribute to a smooth user interaction. The organization of inbox-related initializations within a single function is logical and effective.
Line range hint
745-747: Concise and focused solution initializationThe
solutionInitfunction is admirably concise and focused on a single task: setting up an event listener for submitting a new template. This simplicity and clear purpose make the function easy to understand and maintain.The function's focused approach to handling template submission is commendable. Its brevity and clear intent contribute to code readability.
Line range hint
749-757: Concise style initialization with effective WordPress integrationThe
styleInitfunction is admirably concise and focused on initializing style-related functionality. It efficiently sets up event listeners for image preview actions and integrates well with WordPress hooks for further style editor initialization.The function's focused approach to style-specific functionality and its integration with WordPress hooks demonstrate good practices in plugin development. The conciseness of the function contributes to code clarity and maintainability.
Line range hint
763-823: Comprehensive global settings initializationThe
globalSettingsInitfunction is well-structured and provides a comprehensive setup for global settings functionality. It efficiently sets up event listeners for various settings actions, including license activation, template installation, and other global configurations. The use of event delegation is a good practice for performance.The function's thorough approach to handling various global settings in one place contributes to a centralized and organized codebase. The inclusion of AJAX calls for certain actions, like license activation, is appropriate for providing a smooth user experience.
Line range hint
825-869: Comprehensive export functionality initializationThe
exportInitfunction is well-structured and provides a thorough setup for export-related functionality. It efficiently sets up event listeners for various export actions, including form migration, CSV export, and other export-related tasks. The function demonstrates good practices in handling different export scenarios.The function's comprehensive approach to managing various export functionalities in one place contributes to a centralized and organized codebase. The inclusion of checks for CSV extensions and handling of multiple export types shows attention to detail in user experience.
Line range hint
871-907: Efficient inbox banner initialization with effective AJAX handlingThe
inboxBannerInitfunction is well-structured and focused on initializing inbox banner functionality. It efficiently sets up an event listener for dismissing the banner and handles the AJAX request for banner dismissal. The use of AJAX for asynchronous updates is a good practice for improving user experience.The function's focused approach to banner-specific functionality and its efficient handling of dismissal contribute to a smooth user interaction. The organization of banner-related initializations within a single function is logical and effective.
Line range hint
909-939: Efficient field option updates with AJAX handlingThe
updateOptsfunction is well-implemented and efficiently handles the updating of field options. It makes good use of AJAX for asynchronous updates, supporting both regular fields and product fields. The function appropriately handles different scenarios and updates the UI accordingly.The function's approach to updating field options asynchronously contributes to a smooth user experience. The handling of different field types (regular vs product) demonstrates good flexibility in the code.
Line range hint
941-943: Concise and focused logic removal triggerThe
triggerRemoveLogicfunction is admirably concise and focused on a single task: triggering the removal of conditional logic for a field. It effectively uses jQuery to simulate a click event on the appropriate element.The function's simplicity and clear purpose make it easy to understand and maintain. Its focused approach to handling logic removal is commendable.
Line range hint
945-953: Effective XML download handlingThe
downloadXMLfunction is well-implemented and effectively handles the process of downloading XML data. It constructs the download URL dynamically based on the provided parameters, making it flexible for different use cases.The function's straightforward approach to triggering the XML download contributes to a simple and effective user experience. The use of
location.hreffor initiating the download is an appropriate method in this context.
Line range hint
955-973: Well-implemented WordPress hooks abstractionThe
hooksobject provides a well-structured abstraction over the WordPress hooks API. It offers methods for both adding and applying filters and actions, making it easier to consistently use hooks throughout the plugin.This implementation promotes good practices in WordPress plugin development by encouraging the use of filters and actions. The abstraction can simplify hook usage and potentially improve code readability and maintainability.
Line range hint
975-1010: Well-structured public API for the admin build moduleThe returned object from
frmAdminBuildJSprovides a well-organized public API for the admin build functionality. It effectively exposes initialization functions, utility methods, and other important functionality, making them accessible to other parts of the application.The structure of the public API promotes good encapsulation by only exposing necessary functions and properties. The inclusion of utility functions like
applyZebraStripingandinitModalin the public API allows for their reuse in other contexts, which is a good practice for code reusability.classes/controllers/FrmFormsController.php (1)
Line range hint
1-2: LGTM!The function logic is correct, and the implementation is accurate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
js/formidable_admin.js (1)
Line range hint
8450-8595: Refactor suggestion: Extract getContextualShortcodes() and hideContextualShortcodes() into a separate module.The new functions
getContextualShortcodes()andhideContextualShortcodes()are tightly coupled to the existing shortcode functionality. Consider extracting them into a separate module to improve code organization and maintainability.You can create a new file like
shortcodeUtils.jsand move these functions there. Then import and use them where needed. This will help keep theformidable_admin.jsfile focused on its core responsibilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- classes/controllers/FrmFormsController.php (3 hunks)
- classes/views/shared/mb_adv_info.php (2 hunks)
- js/formidable_admin.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/controllers/FrmFormsController.php
- classes/views/shared/mb_adv_info.php
🧰 Additional context used
🔇 Additional comments (1)
js/formidable_admin.js (1)
8569-8595: Looks good! The new functions provide contextual shortcodes functionality.The
getContextualShortcodes()function returns an array of shortcodes that are contextual to the current input field. This allows showing only relevant shortcodes based on the field context.The
hideContextualShortcodes()function hides the contextual shortcodes in the shortcode list popup. It iterates over the shortcodes returned bygetContextualShortcodes()and adds thefrm_hiddenclass to the corresponding<li>elements.These functions enhance the shortcode functionality by providing a more targeted and contextual experience.
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@AbdiTolesa I tested and verified that shortcodes are added/removed as described in this PR.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
classes/views/frm-forms/mb_insert_fields.php (1)
6-6: LGTM! Consider adding a comment for clarity.The addition of the
$data_attrsvariable is well-implemented and aligns with the PR objective of managing contextual shortcodes. It correctly usesFrmAppHelper::maybe_json_encode()to handle potential JSON data.Consider adding a brief comment explaining the purpose of this variable for better code readability:
// Prepare data attributes for contextual shortcodes $data_attrs = array( 'data-contextual-shortcodes' => FrmAppHelper::maybe_json_encode( array_keys( FrmFormsController::get_contextual_shortcodes() ) ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- classes/controllers/FrmFormsController.php (2 hunks)
- classes/views/frm-forms/mb_insert_fields.php (1 hunks)
- classes/views/shared/mb_adv_info.php (2 hunks)
- js/formidable_admin.js (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/controllers/FrmFormsController.php
- classes/views/shared/mb_adv_info.php
🧰 Additional context used
🔇 Additional comments (4)
classes/views/frm-forms/mb_insert_fields.php (1)
Line range hint
1-13: Overall, the changes look good and align with the PR objectives.The modifications in this file contribute effectively to the management of contextual shortcodes. The implementation is clean, follows good practices, and integrates well with the existing code structure. Great job on improving the shortcode handling functionality!
js/formidable_admin.js (3)
8468-8477: Approved: getContextualShortcodes function looks goodThe
getContextualShortcodesfunction retrieves the contextual shortcodes from the dataset of thefrm_adv_infoelement usingJSON.parse. This is a clean and efficient way to get the contextual shortcodes.
8479-8493: Approved: showContextualShortcodes function implementationThe
showContextualShortcodesfunction iterates over the contextual shortcodes and shows the corresponding shortcode list items by removing thefrm_hiddenclass. The use ofquerySelectorandclosestto find the relevant elements is appropriate.
8495-8507: Approved: hideContextualShortcodes function implementationSimilar to
showContextualShortcodes, thehideContextualShortcodesfunction iterates over the contextual shortcodes and hides the corresponding shortcode list items by adding thefrm_hiddenclass. The logic is clear and concise.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
js/formidable_admin.js (4)
Line range hint
1-50: Consider splitting this large monolithic file into smaller modulesThe file contains a lot of functionality mixed together which makes it harder to maintain. Consider splitting it into logical modules like:
- form-builder.js
- settings.js
- views.js
- etc.
This would improve maintainability and allow for better code organization.
Line range hint
1066-1089: Improve error handling in AJAX requestsThe AJAX error handling is inconsistent. Consider adding proper error handling and user feedback:
postAjax( data, function( response ) { + if (!response || response.error) { + handleAjaxError(response); + return; + } // Success handling + }).fail(function(xhr, status, error) { + handleAjaxError(error); });
Line range hint
1544-1565: Add input validation for field valuesThe field value handling lacks proper validation before being used in DOM manipulation. Add validation to prevent XSS:
function insertContent( contentBox, variable ) { + // Validate input + if (!isValidFieldContent(variable)) { + console.error('Invalid field content'); + return; + } // Rest of function }
Line range hint
2089-2120: Consider using event delegation for dynamic elementsThe event binding could be optimized by using event delegation for dynamically added elements:
- $newFields.on('click', '.frm_delete_field', clickDeleteField); + $builderForm.on('click', '.frm_field_box .frm_delete_field', clickDeleteField);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
js/formidable_admin.js(3 hunks)
🔇 Additional comments (3)
js/formidable_admin.js (3)
Line range hint 3012-3045: LGTM: Well structured form submission handling
The form submission handling is well structured with proper validation and error handling.
8447-8546: Good implementation of contextual shortcode handling
The contextual shortcode functionality is well implemented with clear separation of concerns and good validation.
244-245:
Potential security risk with global variable exposure
The contextual shortcodes are stored in a global variable which could be manipulated. Consider encapsulating this in a closure or module.
- frmAdminJs.contextualShortcodes = getContextualShortcodes();
+ const contextualShortcodes = getContextualShortcodes();
+ Object.freeze(contextualShortcodes);Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
classes/helpers/FrmFieldsHelper.php(1 hunks)
🔇 Additional comments (1)
classes/helpers/FrmFieldsHelper.php (1)
1085-1087: Verify the usage of form_name in the codebase.
Let's verify how this new case is being used to ensure all use cases are handled properly.
Run the following script to check the usage:
✅ Verification successful
Let me gather more specific information about the implementation and usage of this case.
Based on the gathered information, I can now provide a final response:
The form_name case implementation is properly integrated and verified
The implementation is well-tested and used appropriately in the codebase:
- It's used in form templates and shortcodes with proper fallback handling
- Has corresponding test coverage in
test_FrmShortodeHelper.php - Integrates with the existing shortcode processing system alongside other cases like 'sitename' and 'get'
- Implementation follows the established pattern of checking for required attributes (
form_id)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for form_name usage in shortcodes and dynamic values
# Check for form_name usage in PHP files
echo "Checking form_name usage in PHP files..."
rg -l "form_name" --type php
# Check for specific shortcode usage
echo "Checking shortcode usage..."
rg "\[form_name" --type php
# Check for dynamic value references
echo "Checking dynamic value references..."
rg "dynamic_default_values.*form_name" --type php
Length of output: 2026
Script:
#!/bin/bash
# Check the implementation context
echo "Checking the implementation context..."
rg -B 5 -A 5 "case 'form_name':" classes/helpers/FrmFieldsHelper.php
# Check for dynamic value handling
echo "Checking dynamic value handling..."
ast-grep --pattern 'function get_default_value($$$) {
$$$
}'
# Check test cases
echo "Checking test cases..."
rg -C 5 "form_name" tests/phpunit/entries/test_FrmShortodeHelper.php
Length of output: 1593
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
js/formidable_admin.js (7)
244-245: Consider freezing contextual shortcodes object for immutabilityThe
contextualShortcodesobject is assigned directly tofrmAdminJswhich could allow modification. Consider freezing it:- frmAdminJs.contextualShortcodes = getContextualShortcodes(); + frmAdminJs.contextualShortcodes = Object.freeze(getContextualShortcodes());This prevents accidental modifications to the shortcodes configuration.
Line range hint
1012-1032: Add error handling for field group operationsThe field group operations lack proper error handling. Consider adding try-catch blocks and validation:
function getFieldGroupPopup(sizeOfFieldGroup, childElement) { + try { let popup = document.getElementById('frm_field_group_popup'); if (popup === null) { popup = div(); } else { popup.innerHTML = ''; } popup.id = 'frm_field_group_popup'; + if (!sizeOfFieldGroup || sizeOfFieldGroup < 1) { + throw new Error('Invalid field group size'); + } // ... rest of the function + } catch (error) { + console.error('Field group popup error:', error); + return null; + } }This adds validation and error handling to prevent issues with invalid field groups.
Line range hint
2149-2177: Enhance AJAX security and error handlingThe AJAX calls should include better error handling and security measures:
function postAjax(data, success) { + // Validate input + if (!data || typeof success !== 'function') { + console.error('Invalid parameters for AJAX call'); + return; + } const xmlHttp = new XMLHttpRequest(); const params = typeof data === 'string' ? data : Object.keys(data).map( function(k) { return encodeURIComponent(k) + '=' + encodeURIComponent(data[k]); } ).join('&'); xmlHttp.open('post', ajaxurl, true); + xmlHttp.timeout = 30000; // Add timeout xmlHttp.onreadystatechange = function() { if (xmlHttp.readyState > 3 && xmlHttp.status == 200) { let response = xmlHttp.responseText; try { response = JSON.parse(response); } catch (e) { // Response may not be JSON } success(response); } }; + // Add error handling + xmlHttp.onerror = function() { + console.error('AJAX request failed'); + }; + xmlHttp.ontimeout = function() { + console.error('AJAX request timed out'); + }; xmlHttp.setRequestHeader('X-Requested-With', 'XMLHttpRequest'); xmlHttp.setRequestHeader('Content-type', 'application/x-www-form-urlencoded'); xmlHttp.send(params); return xmlHttp; }This adds:
- Input validation
- Request timeout
- Error handling for failed requests
- Timeout handling
Line range hint
1583-1599: Improve form validation consistencyThe form validation could be more robust and consistent:
function validateExport(e) { e.preventDefault(); + const errors = []; let s = false; const $exportForms = jQuery('input[name="frm_export_forms[]"]'); if (!jQuery('input[name="frm_export_forms[]"]:checked').val()) { $exportForms.closest('.frm-table-box').addClass('frm_blank_field'); + errors.push('No forms selected for export'); s = 'stop'; } const $exportType = jQuery('input[name="type[]"]'); if (!jQuery('input[name="type[]"]:checked').val() && $exportType.attr('type') === 'checkbox') { $exportType.closest('p').addClass('frm_blank_field'); + errors.push('No export type selected'); s = 'stop'; } if (s === 'stop') { + // Display all validation errors + if (errors.length) { + displayValidationErrors(errors); + } return false; } e.stopPropagation(); this.submit(); } + function displayValidationErrors(errors) { + const errorContainer = document.createElement('div'); + errorContainer.className = 'frm_error_style'; + errorContainer.innerHTML = errors.join('<br>'); + document.querySelector('.frm_form_settings').prepend(errorContainer); + }This adds:
- Comprehensive error collection
- Centralized error display
- Better user feedback
Line range hint
1847-1875: Optimize DOM operations for better performanceThe DOM operations could be optimized to reduce reflows and improve performance:
function toggleFormOpts() { + // Cache DOM queries + const $form = jQuery(this).closest('form'); + const $dependent = $form.find('.form_' + this.value); + // Use document fragment for batch DOM updates + const fragment = document.createDocumentFragment(); if (value === '') { - jQuery('.hide_form_opt').hide(); + $form.find('.hide_form_opt').hide(); } else { - jQuery('.form_opt_' + value).fadeIn('slow'); + $dependent.fadeIn('slow'); } + // Batch DOM updates + $dependent.find('.frm_field_opts').each(function() { + fragment.appendChild(this.cloneNode(true)); + }); + + $dependent.empty().append(fragment); }This optimization:
- Caches jQuery selectors
- Uses document fragments for batch DOM updates
- Reduces unnecessary DOM operations
Line range hint
2678-2696: Enhance accessibility featuresThe modal and popup functionality needs better accessibility support:
function initModal(id, width) { const $info = jQuery(id); if (!$info.length) { return false; } const dialogArgs = { dialogClass: 'frm-dialog', modal: true, autoOpen: false, closeOnEscape: true, width: width, resizable: false, draggable: false, + // Add accessibility attributes + role: 'dialog', + 'aria-modal': true, + 'aria-labelledby': id + '-title', + 'aria-describedby': id + '-description', open: function() { + // Trap focus within modal + trapFocus(this); jQuery('.ui-dialog-titlebar').addClass('frm_hidden').removeClass('ui-helper-clearfix'); jQuery('#wpwrap').addClass('frm_overlay'); jQuery('.frm-dialog').removeClass('ui-widget ui-widget-content ui-corner-all'); $info.removeClass('ui-dialog-content ui-widget-content'); bindClickForDialogClose($info); } }; + // Add focus trap functionality + function trapFocus(dialog) { + const focusableElements = dialog.querySelectorAll( + 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])' + ); + const firstFocusable = focusableElements[0]; + const lastFocusable = focusableElements[focusableElements.length - 1]; + + dialog.addEventListener('keydown', function(e) { + if (e.key === 'Tab') { + if (e.shiftKey && document.activeElement === firstFocusable) { + e.preventDefault(); + lastFocusable.focus(); + } else if (!e.shiftKey && document.activeElement === lastFocusable) { + e.preventDefault(); + firstFocusable.focus(); + } + } + }); + } }This adds:
- ARIA attributes for screen readers
- Focus trap for keyboard navigation
- Better modal accessibility
Line range hint
1-50: Improve code organization with module patternConsider restructuring the code using a more modular pattern:
+ // Split into modules + const FormidableAdmin = { + UI: { + initModal: function() { /* ... */ }, + showMessage: function() { /* ... */ } + }, + Forms: { + validation: function() { /* ... */ }, + submission: function() { /* ... */ } + }, + Fields: { + dragDrop: function() { /* ... */ }, + validation: function() { /* ... */ } + }, + Ajax: { + post: function() { /* ... */ }, + get: function() { /* ... */ } + } + }; - function frmAdminBuildJS() { + const frmAdminBuildJS = () => {This organization:
- Separates concerns into logical modules
- Makes code more maintainable
- Improves testability
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
js/formidable_admin.js (7)
Line range hint
450-546: Refactor suggestion: Extract contextual shortcode logic into separate moduleThe contextual shortcode handling logic is complex and would benefit from being extracted into its own module. This would improve maintainability and testability.
// Create a ContextualShortcodeManager class class ContextualShortcodeManager { constructor() { this.contextualShortcodes = this.getContextualShortcodes(); } checkContextualShortcode(item) { // Move existing logic here } // Other methods... }
Line range hint
1021-1040: Add error handling for AJAX requestsThe AJAX error handling in
postAjaxis minimal. Consider adding proper error handling and user feedback.xmlHttp.onreadystatechange = function() { + if (xmlHttp.readyState === 4) { - if (xmlHttp.readyState > 3 && xmlHttp.status == 200) { + if (xmlHttp.status === 200) { response = xmlHttp.responseText; try { response = JSON.parse(response); } catch (e) { // The response may not be JSON, so just return it. } success(response); + } else { + console.error('AJAX request failed:', xmlHttp.status); + // Handle error appropriately + } } };
Line range hint
1583-1599: Improve form validation before submissionThe form submission handling lacks comprehensive validation. Consider adding more robust validation before submitting.
function validateFormBeforeSubmit(form) { const requiredFields = form.querySelectorAll('[required]'); let isValid = true; requiredFields.forEach(field => { if (!field.value.trim()) { isValid = false; highlightError(field); } }); return isValid; } function submitBuild() { if (!validateFormBeforeSubmit(this)) { return false; } // Existing submission code... }
Line range hint
2052-2075: Add rate limiting for API requestsThe email collection form submission lacks rate limiting which could lead to abuse. Consider adding request throttling.
const rateLimiter = { lastSubmitTime: 0, minInterval: 2000, // 2 seconds canSubmit() { const now = Date.now(); if (now - this.lastSubmitTime < this.minInterval) { return false; } this.lastSubmitTime = now; return true; } }; function addMyEmailAddress() { if (!rateLimiter.canSubmit()) { return; } // Existing submission code... }
Line range hint
2434-2450: Improve accessibility for modal dialogsThe modal implementation lacks proper ARIA attributes and keyboard navigation support.
function initModal(id, width) { const $info = jQuery(id); if (!$info.length) { return false; } const dialogArgs = { dialogClass: 'frm-dialog', modal: true, autoOpen: false, closeOnEscape: true, width: width || '550px', role: 'dialog', 'aria-modal': true, 'aria-labelledby': id + '-title', // ... other existing options }; // Add keyboard navigation support $info.on('keydown', function(e) { if (e.key === 'Tab') { // Handle tabbing within modal } }); }
Line range hint
2876-2890: Add debouncing for search functionalityThe search content handler should be debounced to prevent excessive DOM updates.
function searchContent() { const debounceDelay = 250; // ms let timeoutId; return function() { const args = arguments; clearTimeout(timeoutId); timeoutId = setTimeout(() => { // Existing search logic performSearch.apply(this, args); }, debounceDelay); }; }
Line range hint
3124-3140: Verify file permissions before uninstallThe uninstall functionality should verify user permissions before proceeding.
function uninstallNow() { if (!userCanUninstall()) { return false; } if (confirmLinkClick(this) === true) { // Existing uninstall code... } return false; } function userCanUninstall() { return typeof frmGlobal !== 'undefined' && frmGlobal.canUninstall === true; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
classes/controllers/FrmFormsController.php(1 hunks)classes/helpers/FrmFieldsHelper.php(1 hunks)js/formidable_admin.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- classes/controllers/FrmFormsController.php
- classes/helpers/FrmFieldsHelper.php
🔇 Additional comments (1)
js/formidable_admin.js (1)
244-245:
Critical: Global variable exposure creates security risk
The contextualShortcodes property is exposed globally on frmAdminJs, which could allow malicious code to modify it. Consider encapsulating this in a closure or module pattern.
- frmAdminJs.contextualShortcodes = getContextualShortcodes();
+ const contextualShortcodes = getContextualShortcodes();
+ Object.freeze(contextualShortcodes);Likely invalid or redundant comment.
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
I think this looks good to merge.
Thanks @AbdiTolesa @truongwp and @lauramekaj1 for working on this!
🚀
Crabcyborg
deleted the
issue-829-show_some_shortcode_items_conditionally
branch
4 participants
Fix https://github.com/Strategy11/formidable-pro/issues/829
Related update: https://github.com/Strategy11/formidable-views/pull/592
Test procedure
...next toTo,From,CCandBCCfields.Advancedtab in the shortcodes modal.Admin emailandDefault from emailshortcodes are available in the list.Contentfield to open the shortcodes popup.Default Msg,Default HTML,Default PlainandForm nameare not under theAdvancedtab.Screen record