Support custom options for autocomplete dropdown#2187
Conversation
WalkthroughThis pull request enhances autocomplete functionality across both PHP and JavaScript components. In the PHP helper, a new method is introduced to conditionally render either a dropdown or an autocomplete input based on the number of options, along with a helper method for extracting option data. An existing method is updated to check for the existence of the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Helper as FrmAppHelper
participant Renderer as Renderer
Caller->>Helper: maybe_autocomplete_options(args)
alt Option count within dropdown_limit
Helper->>Renderer: Render dropdown input
else
Helper->>Helper: Call get_dropdown_value_and_label_from_option for each option
Helper->>Renderer: Render autocomplete input
end
sequenceDiagram
participant User as User/Event
participant DOM as js/admin/dom.js
participant Init as initAutocomplete
participant Hooks as wp.hooks
User->>DOM: Trigger autocomplete initialization (with container)
DOM->>Init: Call initAutocomplete(type, container)
Init->>Init: Determine element source from data attribute
Init->>Hooks: Trigger doAction on clear and select events
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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:
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
🧹 Nitpick comments (4)
classes/helpers/FrmAppHelper.php (2)
1656-1659: Use the correct version for the@sincetag.
The doc block currently uses@since x.x. Please replacex.xwith the actual version number to maintain accurate version tracking.
1725-1745: Rename references from 'autodropdown' to 'autocomplete' for clarity.
The doc block references “autodropdown” within the function description. Renaming to “autocomplete” may maintain consistency with the rest of the code.classes/views/shared/views-info.php (1)
8-25: Limit the number of retrieved posts to improve performance.
Using'post_type' => 'any'withposts_per_pageset to-1can result in large resource usage. Consider adding a limit or a different post retrieval strategy if the dataset is large.js/formidable_admin.js (1)
Line range hint
8382-8448: New functionality for handling field type changesThis adds new functionality to handle field type changes in the form builder:
- Adds a change event handler that checks if a field type is changed
- Shows a "Save and Reload" modal when field type changes
- Provides options to save and reload immediately or cancel
- Improves UX by giving users clear feedback about the impact of changing field types
Consider adding a loading indicator while the page reloads after saving to provide better visual feedback to users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/helpers/FrmAppHelper.php(2 hunks)classes/views/shared/views-info.php(1 hunks)js/admin/dom.js(3 hunks)js/formidable_admin.js(3 hunks)
🔇 Additional comments (8)
classes/helpers/FrmAppHelper.php (2)
1631-1634: Ensure correct handling of $pages_count->publish.
The added check avoids undefined property errors. This looks good to me.
1660-1723: Validate $args['source'] before counting.
In maybe_autocomplete_options(), consider verifying that $args['source'] is an array to avoid potential type errors during the count() check.
js/admin/dom.js (5)
252-252: Check if container is valid before usage.
When calling initSelectionAutocomplete, ensure that container is a valid DOM element or selector to avoid potential runtime errors.
254-256: Chained autocomplete initializations look good.
Calling initAutocomplete for different types in succession is concise. No issues found.
282-290: Handle JSON parsing failures.
When this.dataset.source has invalid JSON, JSON.parse() may throw an error. Consider wrapping it in a try/catch to fail gracefully.
294-294: No issues with referencing source here.
The code properly references the source variable. Looks good.
347-353: Action hook on autocomplete selection looks good.
Calling wp.hooks.doAction( 'frm_autocomplete_select' ) ensures extensibility. Well implemented.
js/formidable_admin.js (1)
688-689: Change from initAutocomplete to initSelectionAutocomplete
The code changes from using frmDom.autocomplete.initAutocomplete to frmDom.autocomplete.initSelectionAutocomplete. This appears to be a more specific and descriptive function name for initializing autocomplete functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/admin/dom.js (1)
340-346: Update version placeholder in documentation.The implementation of the action hooks is good, but the documentation uses placeholder versions ("x.x"). Please update these to reflect the actual version where these hooks will be introduced.
Apply these diffs to update the version:
- * @since x.x + * @since 6.0Also applies to: 354-360
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/helpers/FrmAppHelper.php(2 hunks)js/admin/dom.js(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Inspections
classes/helpers/FrmAppHelper.php
[error] 1666-1666: Spaces must be used for mid-line alignment; tabs are not allowed
[error] 1699-1699: Equals sign not aligned with surrounding assignments; expected 12 spaces but found 1 space
[error] 1711-1711: Equals sign not aligned with surrounding assignments; expected 2 spaces but found 1 space
[error] 1721-1721: End comment for long condition not found; expected '//end if'
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cypress
🔇 Additional comments (4)
js/admin/dom.js (1)
252-257: LGTM! Clean implementation of container-scoped initialization.The changes effectively support initializing autocomplete within specific containers and add support for custom types.
classes/helpers/FrmAppHelper.php (3)
1631-1631: Good defensive programming!Adding the isset() check for the publish property prevents potential PHP notices/warnings if the property is undefined.
1656-1749: Well-structured implementation of autocomplete/dropdown functionalityThe implementation provides a flexible UI that switches between dropdown and autocomplete based on the number of options. Good separation of concerns and parameter handling.
🧰 Tools
🪛 GitHub Actions: Inspections
[error] 1666-1666: Spaces must be used for mid-line alignment; tabs are not allowed
[error] 1699-1699: Equals sign not aligned with surrounding assignments; expected 12 spaces but found 1 space
[error] 1711-1711: Equals sign not aligned with surrounding assignments; expected 2 spaces but found 1 space
[error] 1721-1721: End comment for long condition not found; expected '//end if'
1666-1721:⚠️ Potential issueFix code style issues
Please fix the following code style issues:
- Line 1666: Use spaces instead of tabs for alignment
- Line 1699: Align equals sign with surrounding assignments (12 spaces expected)
- Line 1711: Align equals sign (2 spaces expected)
- Add proper end comment for the if condition
Apply these fixes:
- 'truncate' => false, + 'truncate' => false, - <option value="<?php echo esc_attr( $value_label['value'] ); ?>" <?php selected( $value_label['value'], $args['selected'] ); ?>><?php echo esc_html( $value_label['label'] ); ?></option> + <option value="<?php echo esc_attr( $value_label['value'] ); ?>" <?php selected( $value_label['value'], $args['selected'] ); ?>><?php echo esc_html( $value_label['label'] ); ?></option> - $autocomplete_value = $value_label['label']; + $autocomplete_value = $value_label['label']; + } //end ifLikely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: Inspections
[error] 1666-1666: Spaces must be used for mid-line alignment; tabs are not allowed
[error] 1699-1699: Equals sign not aligned with surrounding assignments; expected 12 spaces but found 1 space
[error] 1711-1711: Equals sign not aligned with surrounding assignments; expected 2 spaces but found 1 space
[error] 1721-1721: End comment for long condition not found; expected '//end if'
|
@truongwp It might be nice to get this merged if it's required for another update, so a core release isn't blocking the other update. It looks like there are some workflow issues at the moment. Do you think you could get this PR ready to get merged soon? Thank you! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/helpers/FrmAppHelper.php (1)
1739-1749: Consider adding input validation.While the implementation is clean, consider adding validation to handle edge cases:
- Validate array keys exist when accessing
$args['value_key']and$args['label_key']- Add type checking for the
$optionparameterprivate static function get_dropdown_value_and_label_from_option( $option, $key, $args ) { + if ( ! isset( $args['value_key'], $args['label_key'] ) ) { + return array( 'value' => '', 'label' => '' ); + } + if ( is_array( $option ) ) { $value = isset( $option[ $args['value_key'] ] ) ? $option[ $args['value_key'] ] : ''; $label = isset( $option[ $args['label_key'] ] ) ? $option[ $args['label_key'] ] : ''; } else { + if ( ! is_scalar( $option ) ) { + return array( 'value' => '', 'label' => '' ); + } $value = $key; $label = $option; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/helpers/FrmAppHelper.php(2 hunks)classes/views/shared/views-info.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/views/shared/views-info.php
🧰 Additional context used
🪛 GitHub Actions: Inspections
classes/helpers/FrmAppHelper.php
[warning] 1-1: phpdoc_types_order issue found. Please reorder the PHPDoc types.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (2)
classes/helpers/FrmAppHelper.php (2)
631-631: LGTM! Defensive programming improvement.The added check for
publishproperty existence helps prevent potential PHP notices when the property is not set.
1663-1727: Well-structured implementation for handling dropdown/autocomplete options!The method provides a robust solution for dynamically switching between dropdown and autocomplete based on the number of options. Key strengths include:
- Proper parameter validation with defaults
- Secure output escaping
- Clean separation of dropdown vs autocomplete logic
- Efficient handling of selected values
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
classes/helpers/FrmAppHelper.php (1)
1657-1662: Consider these improvements to the implementation.
- Update the "x.x" version number in the PHPDoc to reflect the actual version.
- Consider adding error handling for invalid option formats.
- Add input validation for the
$argsparameter./** - * @since x.x + * @since 6.16.3 * * @param array $args Args. See the method for details. + * @throws InvalidArgumentException If required arguments are missing or invalid. */ public static function maybe_autocomplete_options( $args ) { + if ( ! is_array( $args ) ) { + throw new InvalidArgumentException( 'Args must be an array.' ); + } $defaults = array(Also applies to: 1739-1749
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/helpers/FrmAppHelper.php(2 hunks)classes/views/shared/views-info.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/views/shared/views-info.php
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (2)
classes/helpers/FrmAppHelper.php (2)
631-631: LGTM! Improved property access safety.The added check for
isset($pages_count->publish)before comparison improves code robustness by preventing potential errors when accessing object properties.
1663-1727: LGTM! Well-implemented autocomplete/dropdown functionality.The new method provides a flexible solution for rendering either a dropdown or autocomplete input based on the number of options. The implementation includes proper escaping, validation, and separation of concerns.
|
@Crabcyborg I fixed all PHPCS errors. The PHP CS Fixer doesn't give me the line of error so I can't fix it. The Psalm errors should be fixed in another PR. |
2 participants
No description provided.