Show placeholders for separate values in conditional logic dropdowns#2465
Conversation
Clean up trailing spaces and tabs from function declarations and object literals for consistent code formatting.
Add fallback labels for empty dropdown options in field logic, showing the option value or "Untitled" text when a label is missing. Improves documentation and variable declarations in related functions.
Extract admin script strings into reusable get_admin_script_strings() method in FrmAppHelper. Enhance field option display by adding configurable label fallbacks for empty options and improve parameter validation in FrmFieldOption and FrmFieldValueSelector classes.
Add use_value_as_label parameter to FrmFieldOption::print_single_option() method to control when empty labels should display the option value or fallback text. Update FrmFieldValueSelector to use this new parameter for hide_opt fields, improving admin dropdown UX.
Replace _x() call with frm_admin_js.no_label to use existing localized admin script strings for dropdown option fallback text.
- Add FrmAppHelper::str_contains() method with fallback for PHP < 8.0 - Update FrmFieldValueSelector to use compatibility method instead of native str_contains - Fix FrmFieldOption to only use value as label when option_label is truly empty - Ensure compatibility across different PHP versions
…laceholders-in-conditional-logic
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2465 +/- ##
============================================
- Coverage 27.35% 27.32% -0.03%
- Complexity 8710 8734 +24
============================================
Files 140 140
Lines 28750 28862 +112
============================================
+ Hits 7864 7886 +22
- Misses 20886 20976 +90 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
WalkthroughCentralized the "(no label)" text into a public accessor in FrmAppHelper; added an optional $use_value_as_label flag to option rendering and a protector method in FrmFieldValueSelector to control it; and updated admin JS to optionally use option values as visible labels for specific dropdowns. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant AdminUI as Admin UI (browser)
participant JS as admin.js
participant Server as PHP renderer
rect #EEF8FF
AdminUI->>JS: fillDropdownOpts(field)
JS->>JS: getMultipleOpts(fieldId, showValueAsLabel)
alt showValueAsLabel = true
JS-->>AdminUI: options where empty label -> saved value or frm_admin_js.no_label
else
JS-->>AdminUI: options (label/value)
end
end
rect #FFF6EE
AdminUI->>Server: load page (server-rendered dropdown)
Server->>Server: FrmFieldValueSelector::display_dropdown()
Server->>Server: FrmFieldOption::print_single_option(..., $use_value_as_label)
alt $use_value_as_label = true and label empty
Server-->>AdminUI: <option value="saved_value">saved_value or no_label</option>
else
Server-->>AdminUI: <option value="saved_value">option_label</option>
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/models/FrmFieldValueSelector.php (1)
285-287: Don’t drop legitimate "0" valuesLoose comparison treats '0' as empty. Use strict check.
- if ( $value == '' ) { + if ( $value === '' ) { continue; }
♻️ Duplicate comments (1)
classes/models/FrmFieldValueSelector.php (1)
289-291: Address previous PHP 8 str_contains concernSwitching to FrmAppHelper::str_contains resolves compatibility raised earlier. Good fix.
🧹 Nitpick comments (8)
js/src/admin/admin.js (2)
6099-6101: Scope-gated value-as-label: good; tighten selector.Using field.id.includes('frm_field_logic_opt') works, but can match unintended IDs. Prefer a stricter check (e.g., startsWith('frm_field_logic_opt')) to avoid false positives.
Apply:
- let opts = getMultipleOpts( sourceID, field.id.includes( 'frm_field_logic_opt' ) ); + let opts = getMultipleOpts( sourceID, field.id.startsWith( 'frm_field_logic_opt' ) );
6173-6175: Consistent i18n alias for no_label.Use the existing alias (frmAdminJs) for consistency and to satisfy linting without inline disables.
- label = '' !== saved ? saved : frm_admin_js.no_label; // eslint-disable-line camelcase + label = '' !== saved ? saved : frmAdminJs.no_label;classes/models/FrmFieldOption.php (2)
75-78: Docblock clarity: $truncate and third param semanticsTighten the param descriptions to match actual behavior.
- * @param int $truncate Truncate the option label if true. - * @param bool $use_value_as_label Use the option value as the label if true. + * @param int $truncate Max characters to display for the label. + * @param bool $use_value_as_label When true and the label is blank, fall back to the option value or a "(no label)" placeholder.
92-96: Avoid pre-escaping values passed to selected()Pass raw values for reliable comparisons; selected() handles output.
- selected( esc_attr( $selected_value ), esc_attr( $this->saved_value ) ); + selected( $selected_value, $this->saved_value );classes/models/FrmFieldValueSelector.php (1)
281-291: Micro-optimization: compute the flag once outside the loopAvoid repeating str_contains for every option.
- if ( ! empty( $this->options ) ) { - $truncate = isset( $this->truncate ) ? $this->truncate : 25; - - foreach ( $this->options as $key => $value ) { + if ( ! empty( $this->options ) ) { + $truncate = isset( $this->truncate ) ? $this->truncate : 25; + $show_value_as_label = FrmAppHelper::str_contains( $this->html_name, 'hide_opt' ); + + foreach ( $this->options as $key => $value ) { if ( $value == '' ) { continue; } $option = $this->get_single_field_option( $key, $value ); - $option->print_single_option( $this->value, $truncate, FrmAppHelper::str_contains( $this->html_name, 'hide_opt' ) ); + $option->print_single_option( $this->value, $truncate, $show_value_as_label ); } }classes/helpers/FrmAppHelper.php (3)
3524-3531: Replace @SInCE placeholders with the release versionUse the actual plugin version instead of
x.xfor traceability.
4637-4653: Polyfill looks good; keep @SInCE accurateThe helper preserves PHP 7 compatibility. Update
@since x.xto the real version.
3524-3609: Minor: consider caching admin stringsIf this accessor is called frequently, a static cache would avoid rebuilding the array.
public static function get_admin_script_strings() { - global $wp_version; + static $cache = null; + if ( null !== $cache ) { + return $cache; + } + global $wp_version; $admin_script_strings = array( ... ); - return apply_filters( 'frm_admin_script_strings', $admin_script_strings ); + $cache = apply_filters( 'frm_admin_script_strings', $admin_script_strings ); + return $cache; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/helpers/FrmAppHelper.php(2 hunks)classes/models/FrmFieldOption.php(1 hunks)classes/models/FrmFieldValueSelector.php(1 hunks)js/src/admin/admin.js(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
classes/models/FrmFieldValueSelector.php (2)
classes/models/FrmFieldOption.php (1)
print_single_option(81-97)classes/helpers/FrmAppHelper.php (3)
truncate(2788-2835)FrmAppHelper(6-4654)str_contains(4647-4653)
classes/helpers/FrmAppHelper.php (3)
classes/helpers/FrmFieldsHelper.php (2)
FrmFieldsHelper(6-2508)default_unique_msg(275-280)stubs.php (1)
FrmProAppHelper(44-84)classes/helpers/FrmFormsHelper.php (3)
FrmFormsHelper(6-1999)reserved_words(1721-1766)get_no_title_text(1160-1162)
classes/models/FrmFieldOption.php (1)
classes/helpers/FrmAppHelper.php (3)
truncate(2788-2835)FrmAppHelper(6-4654)get_admin_script_strings(3531-3608)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
🔇 Additional comments (6)
js/src/admin/admin.js (2)
6141-6147: JSDoc + signature extension LGTM.Clear doc, safe default. Backward-compatible with existing call sites.
6099-6124: Verify select rendering with image-only options.When the source field uses image-only options, getMultipleOpts may return markup via getImageLabel; option.innerHTML then receives HTML. Browsers typically flatten this to text, but confirm the logic dropdown shows readable text (saved value or “no label”) across supported browsers.
Steps:
- Create a checkbox with images-as-options, blank labels, separate values on.
- Add conditional logic row and open the value dropdown.
- Confirm entries show saved values or the localized “no label”, not empty rows, in Chrome/Firefox/Safari.
classes/models/FrmFieldOption.php (2)
86-90: Good fallback for blank labelsUsing saved value or a localized “(no label)” keeps options discoverable in logic dropdowns.
82-84: Confirm no empty-value options can slip through when using value-as-labelIf $use_value_as_label is true and $this->saved_value is empty, this would render which could duplicate the blank option in selectors. Verify upstream that saved values are never empty in this path.
classes/helpers/FrmAppHelper.php (2)
3516-3521: LGTM: centralizing admin stringslocalize_script() pulling from get_admin_script_strings() reduces duplication.
1-4655: Allprint_single_option()calls use the 3-arg signature and no directstr_contains()usages remain.
…laceholders-in-conditional-logic
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
classes/helpers/FrmAppHelper.php(2 hunks)classes/models/FrmFieldOption.php(1 hunks)classes/models/FrmFieldValueSelector.php(1 hunks)js/src/admin/admin.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- classes/models/FrmFieldOption.php
- classes/models/FrmFieldValueSelector.php
- js/src/admin/admin.js
🧰 Additional context used
🧬 Code graph analysis (1)
classes/helpers/FrmAppHelper.php (3)
classes/helpers/FrmFieldsHelper.php (2)
FrmFieldsHelper(6-2508)default_unique_msg(275-280)stubs.php (1)
FrmProAppHelper(44-84)classes/helpers/FrmFormsHelper.php (3)
FrmFormsHelper(6-1999)reserved_words(1721-1766)get_no_title_text(1160-1162)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cypress
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
🔇 Additional comments (1)
classes/helpers/FrmAppHelper.php (1)
3514-3514: Refactoring approved, but consider past feedback.The refactoring to extract admin script strings into a separate method is clean. However, based on past review comments, there were concerns about this approach granting "too much access" and making future modifications more difficult. Ensure this aligns with the team's final decision on the approach.
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 (2)
classes/helpers/FrmAppHelper.php(2 hunks)classes/models/FrmFieldValueSelector.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/helpers/FrmAppHelper.php
🧰 Additional context used
🧬 Code graph analysis (1)
classes/models/FrmFieldValueSelector.php (2)
classes/models/FrmFieldOption.php (1)
print_single_option(81-97)classes/helpers/FrmAppHelper.php (1)
truncate(2783-2830)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Run PHP Syntax inspection (8.3)
🔇 Additional comments (2)
classes/models/FrmFieldValueSelector.php (2)
290-290: LGTM: Backward-compatible integration with new flag.The addition of the third parameter integrates correctly with the updated
print_single_optionsignature inFrmFieldOption, which defaults tofalsefor backward compatibility.
297-306: Well-designed extension point for Pro.The protected method provides a clean hook for
FrmProFieldValueSelectorto override behavior while keeping the Lite implementation generic. Returningfalseby default ensures no behavior change unless explicitly overridden in a subclass.
Crabcyborg
deleted the
feature/show-option-placeholders-in-conditional-logic
branch
2 participants
This PR adds visual indicators for separate values in conditional logic dropdowns when option labels are not present. Previously, when using checkboxes with separate values but no labels (common with image options), users would see blank lines in the conditional logic dropdown with no way to identify which option was which.
Steps to reproduce
Before
After