Limit fields in group to 12#2575
Conversation
WalkthroughIntroduce MAX_FIELD_GROUP_SIZE = 12 and replace hard-coded Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Admin UI
participant Group as FieldGroupManager
participant Layout as LayoutGenerator
participant Render as Renderer
UI->>Group: add field / request group info
Group->>Group: evaluate capacity (uses MAX_FIELD_GROUP_SIZE = 12)
alt size ≤ 6
Group->>Layout: compute multi-column options (including Custom)
Layout-->>Group: multiple layout options
else size > 6 and ≤ 12
Group->>Layout: request full-width option
Layout-->>Group: single full-width (frm_full / frm1)
end
Group->>Render: render selected layout
Render-->>UI: update display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–30 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
js/src/admin/admin.js (1)
4918-4921: Merge limit still hard-coded to 6 — update to constant.Merging selected groups is blocked when totalFieldCount > 6, which conflicts with the new group cap of 12. Use MAX_FIELD_GROUP_SIZE for consistency, allowing merge up to the same limit.
Apply this diff:
- if ( totalFieldCount > 6 ) { + if ( totalFieldCount > MAX_FIELD_GROUP_SIZE ) { return false; }
🧹 Nitpick comments (3)
js/src/admin/admin.js (3)
4252-4256: Doc index range is outdated.Index can be up to size-1 (now up to 11). Update the JSDoc to avoid confusion.
Apply this diff:
- * @param {number} index 0-5. + * @param {number} index 0-11.
4276-4279: Doc index range in getEvenClassForSize is outdated.Align the JSDoc index to the new max of 11.
Apply this diff:
- * @param {number|undefined} index 0-5. + * @param {number|undefined} index 0-11.
4281-4284: Even class for size > 6 returns frm1 — consider distribution.Returning frm1 is simple but leaves unused grid space for 7–11 fields. Optional: distribute remainder columns (like the special case for 5) to better fill 12 columns.
Would you like a helper that assigns floor(12/size) with the first (12 % size) items getting +1?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(8 hunks)
🧰 Additional context used
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js
[failure] 239-239:
Trailing spaces not allowed
⏰ 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). (7)
- 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)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (5)
js/src/admin/admin.js (5)
2035-2043: Capacity check aligned to constant — LGTM.The max-in-group logic is correct and preserves moving within the same group when at capacity.
2285-2285: Duplicate cap uses constant — LGTM.Using MAX_FIELD_GROUP_SIZE keeps duplication rules consistent with drag/drop limits.
4090-4093: Custom layout hidden for size > 6 — confirm intent.With the group limit raised to 12, Custom layout remains limited to groups ≤ 6. Confirm this UX is intentional and not meant to expand with the new cap.
4160-4164: Single “Even” option for size > 6 — LGTM.The simplified layout choice for larger groups matches the new upper bound.
4199-4205: Preview class for size > 6 — LGTM.Using frm_full on the option wrapper keeps the preview readable while underlying blocks still reflect the computed sizes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
js/src/admin/admin.js (1)
238-239: Remove trailing whitespace (ESLint failure).Line 239 has trailing spaces. Please delete them to satisfy lint rules.
- const MAX_FIELD_GROUP_SIZE = 12; - + const MAX_FIELD_GROUP_SIZE = 12; +
🧹 Nitpick comments (1)
js/src/admin/admin.js (1)
4252-4255: Fix JSDoc index ranges for larger groups.Docs still say “index 0-5” but size now supports 2–12. Update to “index 0..size-1” for both functions.
- * @param {number} index 0-5. + * @param {number} index 0..(size-1).Also applies to: 4275-4279
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(9 hunks)
🧰 Additional context used
🪛 GitHub Check: Run ESLint
js/src/admin/admin.js
[failure] 239-239:
Trailing spaces not allowed
⏰ 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)
- GitHub Check: Cypress
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
🔇 Additional comments (5)
js/src/admin/admin.js (5)
2285-2285: Use of MAX_FIELD_GROUP_SIZE in duplication gate is correct.Nice consolidation; the user-facing modal string already parameterizes the max.
4090-4093: Hiding Custom layout for groups > 6 is consistent with new limit.Good UX guard to avoid fractional/awkward splits.
4919-4921: Merge-capacity guard uses MAX_FIELD_GROUP_SIZE.Good consistency; prevents creating oversized rows during merge.
2034-2043: Capacity check logic is sound; all capacity references use MAX_FIELD_GROUP_SIZE constant.Verification confirms no hard-coded "6" comparisons remain in the file. All capacity checks—at lines 2034–2039 (group fit check), 2284 (duplication limit), and 4918 (merge capacity)—consistently reference the centralized constant. The logic correctly preserves intra-group moves when at max capacity by allowing equal-length rows through to the field-matching filter.
4160-4164: Preview and applied layout are separate concerns; suggested fix breaks intended behavior.The review confuses two distinct mechanisms: the preview option display (which uses
frm_fullfor UI feedback) and the actual field layout applied when users select the "even" option (which usesgetEvenClassForSize). These serve different purposes and should not be conflated.For sizes 7–11, mathematically there is no exact fit in a 12-column grid (12÷7≈1.71, 12÷8=1.5, etc.). The current
frm1(1-column width) allows the grid to auto-wrap fields naturally. The suggested change tofrm12(full-width, 12 columns) would force one field per row, which contradicts the intent of an "even" layout option and would be worse than the current behavior.The preview displaying
frm_fulldoes not indicate the applied layout should also be full-width; it merely shows a UI preview of the option. No evidence demonstrates that the currentfrm1implementation causes actual visual issues or gaps in the rendered form.Likely an incorrect or invalid review comment.
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)
js/src/admin/admin.js (1)
4252-4256: JSDoc: fix index range to match size up to 12.Docs say index 0–5, but with size up to 12, index spans 0–(size−1). Update to prevent confusion.
@@ - * @param {number} index 0-5. + * @param {number} index 0-(size-1). @@ - * @param {number|undefined} index 0-5. + * @param {number|undefined} index 0-(size-1).Also applies to: 4275-4281
♻️ Duplicate comments (1)
js/src/admin/admin.js (1)
239-239: Trailing whitespace — please re-run ESLint.ESLint previously flagged trailing spaces on Line 239. Ensure this blank line has no trailing spaces to pass the lint check.
Run to spot any trailing whitespace:
#!/bin/bash rg -n "[ \t]+$" js/src/admin/admin.js
🧹 Nitpick comments (1)
js/src/admin/admin.js (1)
238-241: De‑magic the “6” layout threshold with a named constant.Multiple places gate layout options on size <= 6 or > 6. Extract a MAX_CUSTOM_LAYOUT_SIZE (6) so future changes are one-line and intent is clear.
@@ - const MAX_FIELD_GROUP_SIZE = 12; + const MAX_FIELD_GROUP_SIZE = 12; + // Upper bound for showing granular/custom row layout options in the UI. + const MAX_CUSTOM_LAYOUT_SIZE = 6; @@ - if ( sizeOfFieldGroup <= 6 ) { + if ( sizeOfFieldGroup <= MAX_CUSTOM_LAYOUT_SIZE ) { @@ - if ( size > 6 ) { + if ( size > MAX_CUSTOM_LAYOUT_SIZE ) { @@ - if ( size < 6 ) { + if ( size < MAX_CUSTOM_LAYOUT_SIZE ) { @@ - switch ( size ) { + switch ( size ) { - case 6: + case MAX_CUSTOM_LAYOUT_SIZE: useClass = 'frm_half'; break; @@ - default: - if ( size > 6 ) { + default: + if ( size > MAX_CUSTOM_LAYOUT_SIZE ) { // We only show a single option at 6-12, so we use the full width. useClass = 'frm_full'; } else { useClass = size % 2 === 1 ? 'frm_fourth' : 'frm_third'; }Also applies to: 4090-4093, 4160-4163, 4172-4179, 4191-4205
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(9 hunks)
⏰ 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). (1)
- GitHub Check: Cypress
🔇 Additional comments (4)
js/src/admin/admin.js (4)
2033-2043: Capacity guard now uses MAX_FIELD_GROUP_SIZE — looks good.The <, >, and == cases correctly allow moves within the same full group and block growth beyond 12. No issues spotted.
2282-2294: Duplicate flow capacity check updated — good.Using MAX_FIELD_GROUP_SIZE in canDuplicateField keeps UX consistent with drag/drop limits.
4916-4921: Merge guard respects MAX_FIELD_GROUP_SIZE — good.selectedFieldsAreMergeable short-circuits when total would exceed 12; consistent with other checks.
4281-4284: Confirm visual intent for >6 fields (“even” -> frm1).For size 7–12, getEvenClassForSize returns 'frm1' per field (1/12 width), which may leave remaining space for sizes <12. If that’s intended (single compact row), all good; otherwise consider wrapping or distributing to fill 12 columns.
Steps:
- Create groups with 7, 8, 9, 10, 11, 12 fields.
- Open Row Layout; confirm only the single option shows and applied layout matches product design for each size (no unintended large gaps, wrapping, or overflow).
Also applies to: 4199-4205, 4160-4163
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)
js/src/admin/admin.js (1)
4251-4291:getEvenClassForSizechange for size >6 likely breaks “full‑width rows” behaviorPreviously, for
size > 6,getEvenClassForSizefell through toreturn 'frm12';, giving each field a full‑width row when auto (“even”) layout was applied. After your change:function getEvenClassForSize( size, index ) { if ( size > 6 ) { return 'frm1'; } // ... }
'frm1'in this codebase represents a 1/12‑width column (seegetSmallClassForSize(6)returning'frm1'andgetSizeOfLayoutClassinterpreting'frm1'as1), so for groups with 7–12 fields:
syncLayoutClasses(..., 'even')will now assignfrm1to every field, making each very narrow instead of stacked full‑width.- The row-layout preview blocks for size >6 will also render as 1/12 columns, conflicting with the “full-width rows” intent expressed in the PR description.
This looks like a behavioral regression rather than just a visual tweak.
Consider reverting this branch to keep the previous full‑width semantics:
function getEvenClassForSize( size, index ) { - if ( size > 6 ) { - return 'frm1'; - } + if ( size > 6 ) { + // For groups larger than 6, treat each field as a full-width row. + return 'frm12'; + }This will keep larger groups constrained by
MAX_FIELD_GROUP_SIZEwhile preserving the existing layout expectations for forms already using 7+ fields in a group.
🧹 Nitpick comments (1)
js/src/admin/admin.js (1)
4155-4213: Row layout options for size >6 mostly align with spec, but rely on hard-coded 6The early
if ( size > 6 ) { wrapper.appendChild( getRowLayoutOption( size, 'even' ) ); return wrapper; }correctly limits 7–12 sized groups to a single “even/full-row” layout option, andgetRowLayoutOption’ssize > 6default usinguseClass = 'frm_full'is consistent with that intent.One thing to flag: the threshold
6is still hard-coded here rather than derived fromMAX_FIELD_GROUP_SIZE. If the max ever changes again, this branch will need a corresponding update or a derived constant (e.g.,MAX_COMPLEX_LAYOUT_SIZE) to avoid drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(9 hunks)
⏰ 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)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (5)
js/src/admin/admin.js (5)
238-243: IntroducingMAX_FIELD_GROUP_SIZEconstant looks goodDefining
MAX_FIELD_GROUP_SIZE = 12here centralizes the group-size limit and is correctly scoped withinfrmAdminBuildJS. No linting or runtime concerns.
2032-2043: Group capacity logic matches new 12‑field limitUpdating
groupCanFitAnotherFieldto useMAX_FIELD_GROUP_SIZEcorrectly enforces the new max while still allowing a move within the same full group (thedata-fidcheck on the final line preserves existing behavior). This should safely cap groups at 12 without regressions in drag/drop behavior.
2282-2293:duplicateFieldnow respects the shared max group sizeUsing
const maxFieldsInGroup = MAX_FIELD_GROUP_SIZE;keeps duplication in sync with drag/drop limits, and the infoModal message will always reflect the actual cap. This avoids the prior “6” magic number divergence.
4090-4095: Custom layout option correctly gated to groups of ≤6 fieldsConditioning
getCustomLayoutOption()onsizeOfFieldGroup <= 6aligns the UI with the intended range for custom layouts and avoids exposing complex layout controls for larger (7–12) groups. This is a sensible UX guardrail.
4905-4923: Merge‑into‑row logic now correctly enforces 12‑field capUsing
MAX_FIELD_GROUP_SIZEinselectedFieldsAreMergeableensures multi‑row merges can’t produce a row with more than 12 fields, consistent with drag/drop and duplication limits. This is an important guardrail to prevent overfull groups via the merge UI.
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@Crabcyborg I tested it and it's working as expected. Thank you! 🚀
|
Thank you Laura! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
js/src/admin/admin.js (1)
4123-4128: Row layout behavior for 7–12 fields and minor JSDoc tweaksThe layout changes for larger groups make sense:
getFieldGroupPopuponly shows “Custom layout” whensizeOfFieldGroup <= 6, which matches the requirement that complex layouts be limited for larger groups.getRowLayoutOptions/getRowLayoutOption/getEvenClassForSizereducesize > 6to a single “even” choice and map those to'frm1'classes, so groups of 7–12 fields still distribute evenly within the 12‑column grid.Two small follow-ups you might consider:
- Visually sanity-check a 7–12 field group in the builder/front-end to confirm the
'frm1'distribution and single “Row Layout” option match design expectations.- Update the JSDoc for
getClassForBlock/getEvenClassForSizeso theindexparameter isn’t documented as only0-5now thatsizecan be up to 12 (e.g., “0–(size‑1)” or similar) to avoid confusion for future maintainers.Also applies to: 4188-4215, 4220-4245, 4284-4290, 4308-4317
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
js/src/admin/admin.js(9 hunks)
⏰ 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)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (1)
js/src/admin/admin.js (1)
238-239: MAX_FIELD_GROUP_SIZE usage and capacity guards look consistentThe introduction of
MAX_FIELD_GROUP_SIZE = 12and its use ingroupCanFitAnotherField,duplicateField(viamaxFieldsInGroup), andselectedFieldsAreMergeablecorrectly centralizes the group-size limit and preserves prior semantics:
- Groups accept new fields while
length < MAX_FIELD_GROUP_SIZE, and reject drops once full, while still allowing reordering within the same group (thedata-fidcheck whenlength === MAX_FIELD_GROUP_SIZE).- Duplication respects the same cap for in-row duplicates, and merge-into-row is prevented once the combined selection would exceed
MAX_FIELD_GROUP_SIZE.I don’t see functional regressions here; the change cleanly scales the previous “6 fields” rules up to 12.
Also applies to: 2057-2068, 2307-2318, 4937-4955
Limit fields in group to 12
2 participants
This update allows users to add up to 12 fields in a group. Up from the previous limit of 6.
I have this set up on my QA site. You can see it here https://qa.formidableforms.com/mike2/wp-admin/admin.php?page=formidable&frm_action=edit&id=937
When there are more than 6 fields in a group, the row layout options are more limited.
And we hide the field ID on hover.
Pre-release
formidable-6.25.3b.zip