enhance: row population option for repeat field #1696
Conversation
Removed redundant dynamic :name bindings from radio field templates in both Vue and PHP templates for consistency. Enhanced admin form builder table styles with new classes for hover, zebra striping, sticky headers/footers, and improved RTL support. Simplified repeat field migration logic in wpuf_get_form_fields to ensure inner_fields is always a simple array, improving compatibility and maintainability.
Refactored the repeat field controls to dynamically create add/remove buttons, ensuring correct visibility and preventing rapid updates. Added MutationObserver to monitor button attribute changes and re-apply visibility logic after field initialization, improving reliability and consistency of repeat field UI in the admin posting interface.
WalkthroughThis PR removes dynamic radio input name bindings in templates, adds a comprehensive Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Potential focus areas:
Possibly related PRs
Suggested labels
Poem
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: 2
🧹 Nitpick comments (9)
assets/js-templates/form-components.php (1)
509-513: Radio group name change: verify uniqueness to avoid cross-group interference.Switching to
:name="option_field.name"is fine if eachoption_field.nameis unique within the open options panel. If two radio option groups share the samename, selecting one will deselect the other at the browser level.
- Action: Verify that no two radio options rendered simultaneously share the same
option_field.name.- Optional hardening: suffix the editing field id to guarantee uniqueness.
Apply if desired:
-:name="option_field.name" +:name="option_field.name + '_' + editing_form_field.id"Also applies to: 524-529
includes/Admin/Posting.php (3)
688-729: Hide “Add” button when max repeats reached.Right now, at max count the “+” button still shows but does nothing. Read
data-max-repeatshere and hide the add button to avoid UX confusion.Apply this diff:
- updateRepeatButtons: function($container) { + updateRepeatButtons: function($container) { var $instances = $container.find('.wpuf-repeat-instance'); var count = $instances.length; + var maxRepeats = parseInt($container.data('max-repeats')) || -1; @@ - // Add button: show only on last instance - if (isLast) { + // Add button: show only on last instance AND when under maxRepeats (if set) + var canAddMore = (maxRepeats === -1) || (count < maxRepeats); + if (isLast && canAddMore) { $controls.append(addButtonHtml); }
607-626: IDs/labels may collide across instances when ids don’t use bracketed indexes.You only rewrite
id/forwhen they contain[...]. If an element’s id uses another scheme (e.g., hyphen-suffixed), clones can end up with duplicate ids and mismatched labels.
- Option: store a
data-base-id/data-base-foron first render and derive unique ids from it during cloning/reindexing.
734-765: MutationObserver scope/targets could be simplified.Observing attribute changes on individual buttons is brittle (buttons are rebuilt) and may miss updates. Either:
- Observe the container with
{childList: true, subtree: true}and call a debouncedupdateRepeatButtons, or- Drop the observer and rely on explicit
updateRepeatButtonscalls (you already call it after add/remove/init).assets/css/admin/form-builder.css (5)
1969-1976: Intersection z-index for pinned header cell.The cell at the intersection of pinned rows and pinned columns should sit above both. Without it, edges can tear during scroll.
Apply this diff right after the sticky header rule:
background-color: var(--fallback-b1,oklch(var(--b1)/var(--tw-bg-opacity))); } +/* Ensure header+pinned intersection stays above */ +.wpuf-table :where(.wpuf-table-pin-rows):where(.wpuf-table-pin-cols) thead tr :is(th, td):first-child { + z-index: 2; +}
927-937: Hover state doesn’t repaint pinned cells.Row hover updates the row background but sticky pinned cells keep their base bg, producing a visual seam. Mirror the hover color onto pinned cells too.
.wpuf-table tr.wpuf-hover:hover, .wpuf-table tr.wpuf-hover:nth-child(even):hover { --tw-bg-opacity: 1; background-color: var(--fallback-b2,oklch(var(--b2)/var(--tw-bg-opacity))); } + + .wpuf-table tr.wpuf-hover:hover :where(.wpuf-table-pin-cols) :is(th, td) { + --tw-bg-opacity: 1; + background-color: var(--fallback-b2,oklch(var(--b2)/var(--tw-bg-opacity))); + } .wpuf-table-zebra tr.wpuf-hover:hover, .wpuf-table-zebra tr.wpuf-hover:nth-child(even):hover { --tw-bg-opacity: 1; background-color: var(--fallback-b3,oklch(var(--b3)/var(--tw-bg-opacity))); } + .wpuf-table-zebra tr.wpuf-hover:hover :where(.wpuf-table-pin-cols) :is(th, td) { + --tw-bg-opacity: 1; + background-color: var(--fallback-b3,oklch(var(--b3)/var(--tw-bg-opacity))); + }
1960-1967: Use logical alignment to simplify RTL.Switch
text-align: lefttotext-align: startso it auto-flips in RTL without extra overrides..wpuf-table { position: relative; width: 100%; border-radius: var(--rounded-box, 1rem); - text-align: left; + text-align: start; font-size: 0.875rem; line-height: 1.25rem; }
2828-2831: Redundant RTL text-align override.If you adopt
text-align: startabove, this RTL-specific override becomes unnecessary. Remove to reduce cascade complexity.-.wpuf-table:where([dir="rtl"], [dir="rtl"] *) { - text-align: right; -}
4081-4084: Confirm.wpuf-table { display: table; }won’t wrap a native<table>.If
.wpuf-tableis applied to a wrapper div around a real<table>, forcing the wrapper todisplay: tablecan cause unexpected box metrics and interfere with layout/overflow. If the class is meant for the<table>element itself, all good; otherwise consider scoping to.wpuf-table table { display: table; }.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
admin/form-builder/assets/js/components/field-radio/template.php(0 hunks)assets/css/admin/form-builder.css(6 hunks)assets/js-templates/form-components.php(2 hunks)includes/Admin/Posting.php(4 hunks)wpuf-functions.php(1 hunks)
💤 Files with no reviewable changes (1)
- admin/form-builder/assets/js/components/field-radio/template.php
🔇 Additional comments (3)
assets/js-templates/form-components.php (2)
24-27: No-op cleanup looks good.Whitespace-only change; no behavioral impact.
123-126: No-op cleanup looks good.Whitespace-only change; no behavioral impact.
assets/css/admin/form-builder.css (1)
2980-3030: Bottom-nav size utilities look good.Consistent height and label typography scale across xs/sm/md/lg; naming matches existing utility patterns.
wpuf-functions.php
Outdated
There was a problem hiding this comment.
Flattening repeat inner_fields drops columns > column-1 (data loss/regression risk).
Current logic keeps only column-1 and discards column-2, column-3, etc. Existing multi-column repeat fields would silently lose inner fields.
- Fix: flatten all column buckets in order instead of only
column-1.
Apply this diff:
- // Ensure inner_fields is a simple array (not column structure)
- if ( empty( $field['inner_fields'] ) ) {
- $field['inner_fields'] = [];
- } elseif ( isset( $field['inner_fields']['column-1'] ) ) {
- // Convert column structure to simple array
- $field['inner_fields'] = $field['inner_fields']['column-1'];
- }
+ // Ensure inner_fields is a simple array (not column structure)
+ if ( empty( $field['inner_fields'] ) ) {
+ $field['inner_fields'] = [];
+ } elseif ( is_array( $field['inner_fields'] ) && isset( $field['inner_fields']['column-1'] ) ) {
+ // Convert column structure to simple array by concatenating all columns
+ $columns = $field['inner_fields'];
+ // keep natural order: column-1, column-2, ...
+ ksort( $columns, SORT_NATURAL );
+ $flat = [];
+ foreach ( $columns as $col => $items ) {
+ if ( is_array( $items ) ) {
+ foreach ( $items as $it ) {
+ $flat[] = $it;
+ }
+ }
+ }
+ $field['inner_fields'] = $flat;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Ensure inner_fields is a simple array (not column structure) | |
| if ( empty( $field['inner_fields'] ) ) { | |
| $old_id = $field['id']; | |
| $old_meta = $field['name']; | |
| $old_label = $field['label']; | |
| $new_id = wpuf_form_field_id_generator(); | |
| $field['template'] = 'text_field'; | |
| // set the new compatible values | |
| $field['id'] = $new_id; | |
| $field['name'] = $old_meta . '_' . $new_id; | |
| $field['label'] = ''; | |
| $field['inner_fields']['column-1'] = [ $field ]; | |
| $field['inner_fields']['column-2'] = []; | |
| $field['inner_fields']['column-3'] = []; | |
| $field['template'] = 'repeat_field'; | |
| $field['columns'] = 1; | |
| $field['min_column'] = 1; | |
| $field['max_column'] = 3; | |
| $field['column_space'] = 5; | |
| $field['id'] = $old_id; | |
| $field['label'] = $old_label; | |
| $field['name'] = $old_meta; | |
| } | |
| // if old repeat field format | |
| if ( empty( $field['inner_columns_size'] ) ) { | |
| $field['inner_columns_size']['column-1'] = '100%'; | |
| $field['inner_columns_size']['column-2'] = '100%'; | |
| $field['inner_columns_size']['column-3'] = '100%'; | |
| $field['inner_fields'] = []; | |
| } elseif ( isset( $field['inner_fields']['column-1'] ) ) { | |
| // Convert column structure to simple array | |
| $field['inner_fields'] = $field['inner_fields']['column-1']; | |
| } | |
| // Ensure inner_fields is a simple array (not column structure) | |
| if ( empty( $field['inner_fields'] ) ) { | |
| $field['inner_fields'] = []; | |
| } elseif ( is_array( $field['inner_fields'] ) && isset( $field['inner_fields']['column-1'] ) ) { | |
| // Convert column structure to simple array by concatenating all columns | |
| $columns = $field['inner_fields']; | |
| // keep natural order: column-1, column-2, ... | |
| ksort( $columns, SORT_NATURAL ); | |
| $flat = []; | |
| foreach ( $columns as $col => $items ) { | |
| if ( is_array( $items ) ) { | |
| foreach ( $items as $it ) { | |
| $flat[] = $it; | |
| } | |
| } | |
| } | |
| $field['inner_fields'] = $flat; | |
| } |
🤖 Prompt for AI Agents
In wpuf-functions.php around lines 1857 to 1863 the current logic flattens
repeat inner_fields by keeping only 'column-1', which drops 'column-2',
'column-3', etc.; change the code to detect a column-bucketed structure and
merge all column-* buckets in numeric order into a single simple array
(preserving field order), e.g. collect keys matching /^column-(\d+)$/, sort by
the captured index, then append each bucket's entries into
$field['inner_fields']; also ensure the existing empty/already-simple-array
branches remain unchanged so non-column structures are not altered.
…lation_option_for_repeat_field
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
assets/css/admin/form-builder.css (1)
2076-2112: Duplicate.wpuf-table/ pinned‑row/col rules and still‑broken pinned‑column behaviorThis block duplicates the earlier
.wpuf-tabledefinitions (lines 2038–2074), including the pinned‑column rule:
.wpuf-table :where(.wpuf-table-pin-cols tr th)sets bothleft: 0andright: 0, which will stretch sticky cells across the horizontal extent and can cause overlap when scrolling.- It only targets
th, so pinned body cells (td) won’t stick with their header.- There’s no explicit stacking context/z‑index on pinned cells, so they may end up under neighbor content.
Given there’s already an earlier copy of the same rules, this both preserves the bug and bloats the CSS.
Consider (a) consolidating to a single
.wpuf-table/pinning block and (b) updating the pinned‑column rules as previously suggested to use a single inline start offset, include boththandtd, and add a stacking context, e.g.:-.wpuf-table :where(.wpuf-table-pin-cols tr th) { - position: sticky; - left: 0px; - right: 0px; - --tw-bg-opacity: 1; - background-color: var(--fallback-b1,oklch(var(--b1)/var(--tw-bg-opacity))); -} - -.wpuf-table-zebra tbody tr:nth-child(even) :where(.wpuf-table-pin-cols tr th) { - --tw-bg-opacity: 1; - background-color: var(--fallback-b2,oklch(var(--b2)/var(--tw-bg-opacity))); -} +.wpuf-table :where(.wpuf-table-pin-cols tr :is(th, td)) { + position: sticky; + inset-inline-start: 0; + z-index: 1; + --tw-bg-opacity: 1; + background-color: var(--fallback-b1,oklch(var(--b1)/var(--tw-bg-opacity))); +} + +.wpuf-table-zebra :where(.wpuf-table-pin-cols) tbody tr:nth-child(even) :is(th, td) { + --tw-bg-opacity: 1; + background-color: var(--fallback-b2,oklch(var(--b2)/var(--tw-bg-opacity))); +}Apply this to whichever single block you keep.
🧹 Nitpick comments (4)
includes/Admin/Posting.php (3)
471-493: Repeat field admin rendering depends entirely on ProField_RepeatclassThe special-case branch for
template === 'repeat_field' && input_type === 'repeat'looks correct and keeps other fields on the existing rendering path. Note that in environments whereWeDevs\Wpuf\Pro\Fields\Field_Repeatis not loaded, repeat fields will silently not render in the admin metabox. If that’s not intended, consider a graceful fallback (e.g., render first row via the generic field object as before).
581-649: Reindexing logic is a bit fragile and has some unused parameters/localsFunctionally this add/clone path will work as long as your repeat field markup follows the expected
[0],[1], … pattern, but there are a few maintainability/robustness issues:
fieldNameis passed intoaddRepeatInstanceandreindexInstancesbut never used.instanceIndexlocals captured on click are never used.- The
name/id/forupdates rely onoriginal*.replace(/\[\d+\]/, '[' + newIndex + ']'), which only rewrites the first numeric bracket. That’s fine for simplefield[0]patterns, but will be a no-op if the first bracket is non‑numeric (e.g.field[key][0]) and may be surprising for more complex nested structures.Consider tightening this by:
- Dropping unused variables/parameters, or wiring
fieldNameinto the selector to avoid over‑eager replacements.- Making the reindexing logic explicitly target the repeat index portion (e.g.,
fieldName[index][…]) instead of “first numeric bracket” so future field templates don’t accidentally break this assumption.
651-687:reindexInstancesworks but could over‑rewrite attributes in complex markup
reindexInstancescorrectly normalizesdata-instance,name,id, andforacross instances, which is key for saving meta reliably. The same/\[\d+\]/replacement is applied to every element that happens to have a[in its attribute, though, which could unintentionally touch nested or unrelated bracketed attributes inside the instance if such markup is ever introduced.If you expect more complex repeat templates in the future, consider scoping the rewrite to attributes that start with the repeat field’s base name (e.g.,
^${fieldName}\[\d+\]) rather than any bracketed segment.assets/css/admin/form-builder.css (1)
3005-3023: Redundant table border/head/foot rulesThe rules for:
.wpuf-table :where(thead tr, tbody tr:not(:last-child), tbody tr:first-child:last-child).wpuf-table :where(thead, tfoot).wpuf-table :where(tfoot)are defined earlier (2981–2999) and then repeated verbatim here. Functionally harmless, but it increases CSS size and makes future edits easier to miss in one copy.
You can safely remove this second block and keep a single source of truth near the primary
.wpuf-tabledefinitions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
admin/form-builder/assets/js/components/field-radio/template.php(0 hunks)assets/css/admin/form-builder.css(2 hunks)assets/js-templates/form-components.php(4 hunks)includes/Admin/Posting.php(7 hunks)wpuf-functions.php(1 hunks)
💤 Files with no reviewable changes (1)
- admin/form-builder/assets/js/components/field-radio/template.php
🚧 Files skipped from review as they are similar to previous changes (2)
- wpuf-functions.php
- assets/js-templates/form-components.php
🔇 Additional comments (4)
includes/Admin/Posting.php (4)
558-579:initRepeatFieldassumes at least one.wpuf-repeat-instanceper containerThe initializer logic is straightforward and the event bindings look good. It does, however, assume that each
.wpuf-repeat-containerhas at least one.wpuf-repeat-instancechild (otherwise later code relying on.first()and.clone()will end up with an empty clone). Please confirm the markup always includes an initial instance; if not, add a guard that creates a first instance template or bails out cleanly.
689-730: Button update guard + MutationObserver: behavior is reasonable but edge‑caseyUsing
$container.data('updating-buttons')to guardupdateRepeatButtonsand then clearing it viasetTimeoutis a pragmatic way to avoid MutationObserver feedback loops. It does, however, mean that multiple updates within that 100ms window will be coalesced into one (or dropped), which might matter if other scripts mutate button classes/styles rapidly.Given the admin‑only context this is probably acceptable; just be aware of the coalescing behavior if you later integrate other code that toggles these controls aggressively.
735-794: MutationObserver wiring for repeat controls looks safeThe
MutationObserverdefinition and initial observation setup are sound, and the globalobserveris in scope for use insideaddRepeatInstance. The additional re‑application ofupdateRepeatButtonsafter initialization (immediate, +100ms, +1000ms) should help keep the UI consistent with async field initializers.No changes needed here; just ensure you’ve smoke‑tested add/remove in a few complex forms (with conditionals, file fields, etc.) to confirm the observer doesn’t cause unexpected reflows.
150-164: Admin field initializer assets: looks consistent, but ensure handle is registeredThe Selectize/intlTelInput +
wpuf-field-initializationenqueue + localization look fine for the admin metabox context; just confirm that thewpuf-field-initializationhandle is registered (with its dependencies on selectize/intlTelInput) before this runs so the script and localized object are reliably available.
Rubaiyat-E-Mohammad
added
QA Approved
2 participants
part of #1038
related PR
Currently, the repeat field is supporting/repeating the whole field set, we are now supporting a single row-wise repeat of fields.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor