Lite Version: Add Format Settings #1864
Conversation
![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)
classes/models/fields/FrmFieldNumber.php (1)
68-83: LGTM! Consider enhancing error messages with actual limits.The code restructuring improves readability and follows good practices with early returns. However, the error messages could be more helpful to users.
Consider including the actual limits in the error messages:
- $errors[ 'field' . $args['id'] ] = __( 'Please select a higher number', 'formidable' ); + $errors[ 'field' . $args['id'] ] = sprintf( + __( 'Please select a number greater than or equal to %s', 'formidable' ), + $minnum + ); - $errors[ 'field' . $args['id'] ] = __( 'Please select a lower number', 'formidable' ); + $errors[ 'field' . $args['id'] ] = sprintf( + __( 'Please select a number less than or equal to %s', 'formidable' ), + $maxnum + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/fields/FrmFieldNumber.php(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/models/fields/FrmFieldNumber.php (1)
1-183: Consider moving format-related functionality to Pro version.Based on the past review comments and the PR objectives mentioning format settings, this functionality might be better suited in
FrmProFieldNumber.phpto maintain consistency with how other custom formats (masking/regex) are handled.Let's verify if there are any format-related methods in this file that should be moved:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/helpers/FrmCurrencyHelper.php (1)
29-37: Consider making format values configurable and clarifying method name.The implementation is clean and uses proper type checking. However, consider these improvements:
- Define format values as class constants for better maintainability.
- Rename method to better reflect its purpose since it accepts both 'currency' and 'number' formats.
Here's a suggested implementation:
+ const VALID_FORMATS = ['currency', 'number']; - public static function is_currency_format( $format_value ) { - return ! empty( $format_value ) && in_array( $format_value, array( 'currency', 'number' ), true ); + public static function is_valid_number_format( $format_value ) { + return ! empty( $format_value ) && in_array( $format_value, self::VALID_FORMATS, true ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/helpers/FrmCurrencyHelper.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (1)
classes/helpers/FrmCurrencyHelper.php (1)
303-303: LGTM! Correct symbol for Turkish Lira.The change fixes a bug by replacing the Euro symbol (€) with the correct Turkish Lira symbol (₺).
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
js/formidable_admin.js (1)
8535-8536: Sanitize shortcode box inputsThe showShortcodeBox function should sanitize input values to prevent XSS attacks.
function showShortcodeBox( moreIcon, shouldFocus ) { + const sanitizedInput = purifyHtml( getInputForIcon( moreIcon ) ); - let input = getInputForIcon( moreIcon ); + let input = sanitizedInput; // ... rest of function }
🧹 Nitpick comments (3)
js/formidable_admin.js (3)
5431-5443: Add input validation for format input updatesThe
maybeUpdateFormatInputfunction should validate the format type value before updating the format input to prevent potential injection attacks.function maybeUpdateFormatInput( event ) { const formatElement = event.target; + const allowedTypes = [ 'custom', 'international', 'currency', 'number' ]; const type = formatElement.value; + if ( !allowedTypes.includes( type ) ) { + return; + } // ... rest of function }
7711-7719: Enhance error handling with specific messagesThe error handling in
handleInsertFieldErrorcould be improved by providing more specific error messages based on the error type.function handleInsertFieldError( jqXHR, _, errorThrown ) { + let errorMessage = 'An error occurred while inserting the field. '; + if ( jqXHR.responseJSON && jqXHR.responseJSON.message ) { + errorMessage += jqXHR.responseJSON.message; + } else { + errorMessage += errorThrown; + } maybeShowInsertFieldError( errorMessage, jqXHR ); }
10701-10702: Improve format dropdown change handlerThe format dropdown change handler should be debounced to prevent rapid consecutive updates.
- frmDom.util.documentOn( 'change', 'select.frm_format_dropdown, select.frm_phone_type_dropdown', maybeUpdateFormatInput ); + frmDom.util.documentOn( 'change', 'select.frm_format_dropdown, select.frm_phone_type_dropdown', debounce( maybeUpdateFormatInput, 250 ) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cypress
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
stubs.php (1)
158-160: Add PHPDoc documentation for the method.Please add PHPDoc documentation to describe the parameters and return type of the
normalize_formatted_numbersmethod.class FrmProCurrencyHelper { + /** + * @param array $field + * @param string $formatted_value + * @return string + */ public static function normalize_formatted_numbers( $field, $formatted_value ) {} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
classes/controllers/FrmSettingsController.php(1 hunks)classes/helpers/FrmCurrencyHelper.php(2 hunks)classes/models/FrmEntryValidate.php(1 hunks)classes/views/frm-settings/general.php(3 hunks)js/admin/settings.js(2 hunks)stubs.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- js/admin/settings.js
- classes/views/frm-settings/general.php
- classes/models/FrmEntryValidate.php
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cypress
🔇 Additional comments (5)
classes/helpers/FrmCurrencyHelper.php (2)
29-37: LGTM!The method is well-documented and correctly implemented with strict comparison.
303-303: LGTM!The Turkish Lira symbol has been correctly updated from Euro (€) to TRY (₺).
classes/controllers/FrmSettingsController.php (1)
209-217: LGTM!The method has been improved to prioritize the Pro version's currency selector while maintaining backward compatibility with a clean fallback implementation.
stubs.php (2)
427-427: LGTM!The method has proper PHPDoc documentation and follows stub file conventions.
428-432: LGTM!The method has proper PHPDoc documentation and follows stub file conventions.
|
@shervElmi It looks like there are issues with failing unit tests now. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
css/frm_admin.css (2)
321-324: Improve specificity for phone field format label opacityThe selector could be simplified and made more specific to avoid potential conflicts.
-.frm_form_field.frm-phone-type ~ [id*="frm-field-format-custom-"] > label, -.frm_form_field.frm-format-dropdown ~ [id*="frm-field-format-custom-"] > label { +.frm_form_field[class*="frm-"] ~ [id*="frm-field-format-custom-"] > label { opacity: 0; }
2059-2069: Consider using a more specific selector for hidden labelsThe current selector list is quite long and could potentially cause specificity issues.
-.frm-fields p > label.frm_hidden, -#wpbody-content label.frm_hidden, -.frm-lookup-modal .dismiss, -.frm-right-panel .inside a.frm_hidden, -#form_global_settings .frm_hidden, -ul.frm_form_nav > li.frm_hidden, -a.frm_hidden, -.button.frm_hidden, -.wp-core-ui .button.frm_hidden { +[class*="frm"] .frm_hidden, +.wp-core-ui .frm_hidden { display: none; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/helpers/FrmAppHelper.php(2 hunks)css/frm_admin.css(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/helpers/FrmAppHelper.php
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (6)
css/frm_admin.css (6)
316-319: LGTM: Grid column span for followed elementsThe CSS rule correctly handles the grid layout for elements following
.frm12_followedclass by making them span 12 columns with!importantto ensure the rule takes precedence.
326-328: LGTM: Grid column span for format dropdownThe CSS rule appropriately handles the grid layout for elements following
.frm-format-dropdownclass by making them span 6 columns.
330-333: LGTM: Full width CSS layout classes with format settingThe CSS rule correctly adjusts the layout for CSS classes when the format setting is present in textarea fields.
1790-1793: LGTM: Vertical margin for extra small spacingThe CSS rule appropriately sets consistent vertical margins using the
--gap-xsCSS variable.
1933-1936: LGTM: Inline flex display for elementsThe CSS rule correctly sets
inline-flexdisplay for elements and overrides WordPress admin styles using#wpbody-contentselector.
2012-2014: LGTM: Flex wrap for wpbody-contentThe CSS rule appropriately enables flex wrapping for elements within
#wpbody-content.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/fields/FrmFieldNumber.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (2)
classes/models/fields/FrmFieldNumber.php (2)
97-97: LGTM! Good visibility change for extensibility.The change from
privatetoprotectedvisibility forvalidate_stepmethod enables better extensibility by allowing subclasses to access and potentially override the step validation logic.
68-82: Verify validation order and Pro version compatibility.Two concerns:
- The min/max validation at line 76 might process invalid numbers that failed the numeric check at line 64. Consider moving the numeric validation after the empty check.
- Based on past review comments, format options might need Pro version compatibility checks.
Run this script to check Pro version handling in similar fields:
#!/bin/bash # Description: Check how other field types handle Pro version compatibility # Search for Pro version checks in field classes rg -A 5 "FrmProFieldsHelper|FrmProAppHelper" "classes/models/fields/" # Search for format option handling ast-grep --pattern 'function show_format_option($$$) { $$$ }'
There was a problem hiding this comment.
I think this is good to merge now.
I'll keep testing, but anything else that comes up can happen in a new PR.
Thank you @shervElmi!
🚀
This update introduces a new feature that adds Format settings to the following field types: Text, Paragraph, Hidden, Slider, and Number.
The feature provides users with the ability to:
This enhancement ensures seamless integration with existing functionality and does not disrupt the experience for existing users.
For a detailed demonstration, please watch the video:
https://share.cleanshot.com/K8RsgbTz
Pro version PR:
https://github.com/Strategy11/formidable-pro/pull/5195
QA URL:
https://qa.formidableforms.com/sherv7/wp-admin/admin.php?page=formidable&frm_action=edit&id=19