Work on adding show options to code list pop up#1379
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1379 +/- ##
=========================================
Coverage 29.26% 29.27%
Complexity 7263 7263
=========================================
Files 105 105
Lines 23937 23937
=========================================
+ Hits 7006 7007 +1
+ Misses 16931 16930 -1 ☔ View full report in Codecov by Sentry. |
|
@AbdiTolesa I was working on adding support for Do you think you'd be able to try taking this over and wrapping it up? I was hoping to also include Thank you! |
WalkthroughAdded three helper functions to centralize creation and insertion of calculation-field list items and to conditionally add label and name-part shortcodes when populating the calculation-field popup in the form builder UI. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Calc Field Popup UI
participant Admin as js/src/admin/admin.js
participant Helpers as js/formidable_admin.js
UI->>Admin: populateCalcFieldList(fields)
Admin->>Helpers: evaluate each field
alt eligible field
Helpers->>Helpers: addCalcFieldLiToList(list, fieldId, code, label, fieldType)
opt show label shortcode
Helpers->>Helpers: add "[fieldId show=label]"
end
opt name field
Helpers->>Helpers: maybeAddNamePartShortcodes(field, fieldId, list)
end
Helpers-->>Admin: appended items
else skipped
Helpers-->>Admin: skip field
end
Admin-->>UI: render populated list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 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. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (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. 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: 2
📜 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)
2905-2907: Ensure browser compatibilityThe
shouldShowFieldLabelShortcodefunction uses optional chaining (?.) andincludes, which are relatively modern JavaScript features.Verify that the browsers supported by the Formidable Forms plugin are compatible with these features, or consider providing polyfills or fallback mechanisms for older browsers if necessary.
#!/bin/bash # Check browser compatibility for optional chaining and includes ast-grep --lang js --pattern $' $$$ (_) ?. (_) $$$ ' ast-grep --lang js --pattern $' $$$ (_).includes(_) $$$ '
2895-2907: Verify compatibility with existing functionalityThe new
shouldShowFieldLabelShortcodefunction introduces new logic for determining whether to show the field label shortcode based on the field type and "Use separate values" checkbox state.Ensure this change is thoroughly tested and does not introduce any regressions or compatibility issues with existing functionality related to calculation fields.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
js/formidable_admin.js (1)
Line range hint
1-3070: Good use of module pattern, but consider breaking it downThe
frmAdminBuildJSfunction implements the module pattern, which is a good practice for encapsulation and organizing code. It creates a closure that contains private functions and variables, while returning an object with public methods and properties. This approach helps in maintaining a clean global namespace and controlling access to internal functionality.However, the function is quite large and contains many nested functions and variables. Consider breaking this down into smaller, more focused modules or sub-modules. This could improve readability, maintainability, and make it easier to test individual components.
Here's a high-level suggestion for refactoring:
- Identify logical groupings of functionality (e.g., form building, settings, export/import).
- Create separate modules for each group.
- Use ES6 modules or a similar system to manage dependencies between these modules.
- Maintain a main module that initializes and coordinates these sub-modules.
This approach would make the code more modular and easier to maintain in the long run.
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)
- js/formidable_admin.js (1 hunks)
🧰 Additional context used
🪛 Biome
js/formidable_admin.js
[error] 2867-2868: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
js/formidable_admin.js (2)
2893-2912: Looks good! The new function adds useful functionality.The
maybeAddNamePartShortcodesfunction is well-structured and adds the ability to insert[nameFieldId show=first],[nameFieldId show=middle]and[nameFieldId show=last]shortcodes to the calculation popup for name fields.It uses
Object.entriesandforEachto efficiently loop through the name parts and their labels, and callsaddCalcFieldLiToListto add the shortcode<li>elements.The function is also properly scoped to only run for 'name' field types.
2937-2959: Looks good! The new function improves code modularity.The new
addCalcFieldLiToListfunction encapsulates the logic for constructing and appending the field shortcode<li>elements to the calculation popup.It takes the necessary parameters and uses them to build the
<a>and<li>elements with the appropriate classes and attributes.Extracting this logic into a separate function improves code modularity and reusability. The function is also well-documented with JSDoc comments explaining the parameters and return value.
…s://github.com/Strategy11/formidable-forms into work_on_adding_show_options_to_code_list_popup
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
js/formidable_admin.js (3)
2862-2884: LGTM! Consider extracting option validation logicThe code for handling excluded options and field validation looks good. For improved readability, consider extracting the option validation logic into a separate helper function.
function validateFieldOption(field, excludedOpts) { return excludedOpts.some(opt => { const inputs = document.getElementsByName(getFieldOptionInputName(opt, field.fieldId)); return inputs.length && jQuery(inputs[0]).val(); }); }🧰 Tools
🪛 Biome (1.9.4)
[error] 2872-2873: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
2933-2964: Consider adding parameter type documentationThe
addCalcFieldLiToListfunction is well-implemented but could benefit from more detailed parameter documentation. Consider adding TypeScript-style JSDoc parameter types:/** * Adds a calculation field list item * @param {HTMLElement} list - The list element to append to * @param {string} fieldId - The field ID * @param {string} code - The field code * @param {string} label - The field label * @param {string} fieldType - The field type */ function addCalcFieldLiToList(list, fieldId, code, label, fieldType) {
2872-2873: Use optional chaining for safer array accessConsider using optional chaining to make the array access safer:
if ( fieldOptions?.length < 2 ) { return; }This change would prevent potential errors if
fieldOptionsis undefined.🧰 Tools
🪛 Biome (1.9.4)
[error] 2872-2873: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
js/formidable_admin.js
[error] 2872-2873: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run PHP Syntax inspection (8.3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
js/formidable_admin.js (1)
2887-2931: Well-structured name field handling with good documentationThe implementation of name part shortcodes is clean and well-documented. Good use of JSDoc comments and modern JavaScript features.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
js/formidable_admin.js (1)
2967-2987: 🛠️ Refactor suggestionAdd error handling for JSON parse
The getExcludeArray function parses JSON without error handling.
Add try/catch block around JSON.parse:
- const exclude = JSON.parse(box.getElementsByClassName('frm_code_list')[0].getAttribute('data-exclude')); + let exclude = []; + try { + const excludeAttr = box.getElementsByClassName('frm_code_list')[0].getAttribute('data-exclude'); + exclude = excludeAttr ? JSON.parse(excludeAttr) : []; + } catch (e) { + console.error('Error parsing exclude data:', e); + }
♻️ Duplicate comments (1)
js/formidable_admin.js (1)
2929-2931: 🛠️ Refactor suggestionFix potential undefined return in shouldShowFieldLabelShortcode
The optional chaining operator could return undefined instead of a boolean.
Add an explicit boolean conversion:
- return [ 'radio', 'checkbox', 'dropdown' ].includes( fieldType ) && document.getElementById( `separate_value_${fieldId}` )?.checked; + return [ 'radio', 'checkbox', 'dropdown' ].includes( fieldType ) && !! document.getElementById( `separate_value_${fieldId}` )?.checked;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/formidable_admin.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
js/formidable_admin.js
[error] 2872-2873: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cypress
🔇 Additional comments (3)
js/formidable_admin.js (3)
2877-2884: LGTM: Proper field list handlingThe field list generation and shortcode support check is implemented correctly. The code properly checks for the data-supportsShowShortcodes attribute before adding additional shortcode options.
2898-2917: LGTM: Well-structured name field shortcode handlingThe maybeAddNamePartShortcodes function is well-implemented with:
- Clear type checking
- Proper field ID validation
- Localized labels using __()
- Clean iteration over name parts
2942-2965: LGTM: Well-structured list item creationThe addCalcFieldLiToList function follows best practices:
- Uses template literals for string interpolation
- Properly escapes HTML content
- Uses semantic HTML elements
- Includes proper data attributes
|
@AbdiTolesa Sorry I let this PR get so old. We've updated the admin JS now so it's in a different admin.js file, and this file that's being changed is minified now. So we'll need to move these changes into the new file. Could you help with that? Thank you! |
@Crabcyborg I have moved the changes to the source js files both in this PR and https://github.com/Strategy11/formidable-pro/pull/5394 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
js/src/admin/admin.js (4)
3050-3065: Feature flag truthiness can misfire; make it explicitly boolean
dataset.supportsShowShortcodesis a string and any non-empty value (including "0") is truthy. Prefer a presence or explicit boolean parse to avoid accidental enabling.-const supportsShowShortcodes = document.getElementById( 'new_fields' ).dataset.supportsShowShortcodes; +const newFieldsEl = document.getElementById( 'new_fields' ); +const supportsShowShortcodes = newFieldsEl?.hasAttribute( 'data-supports-show-shortcodes' ) && + (newFieldsEl.dataset.supportsShowShortcodes === '' || + /^(1|true|yes)$/i.test( newFieldsEl.dataset.supportsShowShortcodes ));
3113-3145: Minor UI polish: add spacing and accessible label to shortcode itemsCurrently the label text is concatenated directly after “[code]” with no space, and the link lacks an accessible label. Add a space node and
aria-labelto improve readability/AT support.function addCalcFieldLiToList( list, fieldId, code, label, fieldType ) { const anchor = a( { className: 'frm_insert_code', children: [ span( { text: '[' + code + ']' } ), - document.createTextNode( label ) + document.createTextNode( ' ' ), + document.createTextNode( label ) ], data: { code - } + }, + ariaLabel: label + ' shortcode ' + '[' + code + ']' } );
3068-3097: Scope OK; consider reflecting actual Name subparts to avoid dangling optionsNice addition. If practical, hide parts not enabled in the Name field’s current layout (e.g., no Middle) to reduce confusion in the popup.
Please confirm if the Name layout setting is available in DOM (eg, a per-field dropdown). If so, I can wire a small check to only add present parts.
3068-3097: Doc nit: replace “@SInCE x.x” with the release versionUpdate the JSDoc
@sincetags to the actual version used in this release.Also applies to: 3099-3112, 3113-3145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
js/src/admin/admin.js (1)
js/admin/dom.js (5)
__(4-4)label(366-372)label(740-740)anchor(710-710)a(27-33)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
js/src/admin/admin.js (1)
3096-3109: Dropdowns won’t show “label” shortcode — include 'select' in type checkThe field type array omits 'select' (used for Dropdowns elsewhere), so [fieldId show=label] never appears for Dropdown fields.
Apply:
-function shouldShowFieldLabelShortcode( fieldType, fieldId ) { - return [ 'radio', 'checkbox', 'dropdown' ].includes( fieldType ) && !! document.getElementById( `separate_value_${ fieldId }` )?.checked; -} +function shouldShowFieldLabelShortcode( fieldType, fieldId ) { + // Support actual Dropdown type ('select'); keep 'dropdown' for safety if referenced elsewhere. + return [ 'radio', 'checkbox', 'select', 'dropdown' ].includes( fieldType ) && + !! document.getElementById( `separate_value_${ fieldId }` )?.checked; +}Please also confirm this UI‑side check aligns with Pro behavior (comment noted that Pro needs no changes). [Based on past review comments]
🧹 Nitpick comments (3)
js/src/admin/admin.js (3)
3110-3141: Minor UI nit: add spacing between “[code]” and labelCurrently renders as “[123]My Field”. Add a leading space to the label text node.
- document.createTextNode( label ) + document.createTextNode( ' ' + label )
3064-3095: Name parts: consider hiding “Middle” when not enabled on the fieldYou always add first/middle/last. If the Name field is configured without a middle part, showing “(Middle)” may confuse. Optional: gate on the field’s current layout/settings (if available) before adding that shortcode. I can draft a DOM check if you confirm where the layout is stored.
3064-3141: Replace @SInCE x.x placeholdersDocblocks use “@SInCE x.x”. Replace with the actual next version tag used in this repo to keep docs accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
js/src/admin/admin.js (1)
js/admin/dom.js (5)
__(4-4)label(366-372)label(740-740)anchor(710-710)a(27-33)
⏰ 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: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
- GitHub Check: Run PHP Syntax inspection (8.3)
- Added maybeAddAddressPartShortcodes function to support Address field part shortcodes (line1, line2, city, state, zip, country) - Added function call in field population loop to display Address field parts in calculation popup - Similar pattern to Name field implementation Resolves #5825
- Use template literals instead of string concatenation - Extract addressParts as const variable for better readability - Add descriptive JSDoc parameter documentation - Refine function description to be more professional and direct - Cleaner forEach syntax with consistent formatting
…hortcodes - Remove hyphens from JSDoc parameter descriptions for consistency with project style - Align parameter documentation format with standard JSDoc conventions
- Added wp.hooks.doAction('frm_add_calc_field_shortcodes') hook
- Removed maybeAddAddressPartShortcodes() function (Address fields are Pro-only)
- Allows Pro and other extensions to add custom field part shortcodes
- Well-documented hook with JSDoc for extension developers
…ptions Add Address field part shortcodes to calculation popup
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
js/src/admin/admin.js (2)
3056-3074: Localize the “(Label)” suffix in label shortcode entry and avoid hard‑coded EnglishThe UI label for the
[fieldId show=label]entry is built asfields[ i ].fieldName + ' (Label)', which is not translatable. This should use the existing i18n helpers so the suffix is localized and consistent with the rest of the admin UI.A minimal change inline with existing patterns:
- if ( shouldShowFieldLabelShortcode( fields[ i ].fieldType, fields[ i ].fieldId ) ) { - addCalcFieldLiToList( list, fieldId, fields[ i ].fieldId + ' show=label', fields[ i ].fieldName + ' (Label)', fields[ i ].fieldType ); - } + if ( shouldShowFieldLabelShortcode( fields[ i ].fieldType, fields[ i ].fieldId ) ) { + addCalcFieldLiToList( + list, + fieldId, + fields[ i ].fieldId + ' show=label', + sprintf( __( '%s (Label)', 'formidable' ), fields[ i ].fieldName ), + fields[ i ].fieldType + ); + }This keeps the UX the same while making the suffix properly translatable.
3119-3121: Dropdowns won’t ever show[fieldId show=label]— type check is missing'select'
shouldShowFieldLabelShortcodeonly allows['radio', 'checkbox', 'dropdown']. In this codebase Dropdown fields actually use the type string'select'elsewhere, so real Dropdown fields will never satisfy this check and theirshow=labelentries won’t appear in the popup.Recommend broadening the type list so Dropdowns are correctly supported:
-function shouldShowFieldLabelShortcode( fieldType, fieldId ) { - return [ 'radio', 'checkbox', 'dropdown' ].includes( fieldType ) && !! document.getElementById( `separate_value_${ fieldId }` )?.checked; -} +function shouldShowFieldLabelShortcode( fieldType, fieldId ) { + // Support actual Dropdown type ('select'); keep 'dropdown' in case it exists elsewhere. + return [ 'radio', 'checkbox', 'select', 'dropdown' ].includes( fieldType ) && + !! document.getElementById( `separate_value_${ fieldId }` )?.checked; +}This aligns the new feature with existing field-type conventions and ensures
show=labelis available for Dropdowns when “Use separate values” is enabled.
🧹 Nitpick comments (3)
js/src/admin/admin.js (3)
3077-3107: Name-part shortcodes helper looks good; consider aligning with actual name layout (optional)
maybeAddNamePartShortcodescleanly adds[nameFieldId show=first|middle|last]entries and correctly uses__()for the sublabels, which matches the PR intent.If you want to tighten UX later, you could read the Name field’s configured layout (eg, which parts are enabled) and skip generating entries for disabled parts so you don’t offer shortcodes like
show=middlewhen no middle-name subfield will ever render. That would avoid confusing, non-functional options in the popup.
3132-3155: Helper for calc-field list items is solid; consider minor accessibility polish
addCalcFieldLiToListcentralizes the markup for calc-field entries and correctly stores the raw shortcode in adata-codeattribute, which makes the new hook API easier to use and keeps the DOM structure consistent.Two small optional tweaks you might consider:
- Add an
href="#"(orrole="button"andtabindex="0") to the<a>so it’s keyboard-focusable and announced as interactive even if JS fails.- If you expect add-ons to call this with non-string
fieldId, you might document/normalize it to a string to avoid surprising class names.No blockers here; helper is otherwise well-factored.
3062-3074: Replace@since x.xplaceholders before releaseThe new
frm_add_calc_field_shortcodeshook and helper docblocks still use@since x.x. Before tagging a release, these should be updated to the actual version number so downstream developers have accurate version metadata in the public JS API.Purely documentation/i18n hygiene; no functional impact.
5 participants
Related update https://github.com/Strategy11/formidable-pro/pull/4573 (merged)
Requires: https://github.com/Strategy11/formidable-pro/pull/5394
TODO
Test procedure
show=firstshow=labeloption is available for the Dropdown field.