Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for “dynamic” grid row counts where a separate controlling field determines how many grid rows are rendered (and the grid behaves as fixed / non-user-addable).
Changes:
- Introduces
dynamic_rows_fieldgrid config option and precedence rules vsfixed_rowsandgrid_rowstag override. - Updates grid field template + JS to initialize and reactively adjust row counts, including add/remove animations.
- Adds feature tests asserting rendered output for dynamic grid behavior and precedence rules.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Feature/FieldTypes/GridFieldTest.php | Adds tests around dynamic grid row rendering, hiding buttons, and precedence behavior. |
| src/Tags/Concerns/HandlesFields.php | Adds dynamic_rows_field processing and updates is_fixed/precedence behavior. |
| src/ServiceProvider.php | Exposes dynamic_rows_field as a configurable grid option in fieldtype config. |
| resources/views/form/fieldtypes/_grid.antlers.html | Adds dynamic row initialization hooks and in-template CSS for row animations. |
| resources/js/formFields.js | Implements dynamic row initialization, reactive row count updates, and enter/exit animations. |
| dist/js/easy-forms.js | Compiled JS output reflecting new dynamic grid logic. |
| dist/css/easy-forms.css | Compiled CSS output update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
resources/js/formFields.js
Outdated
| row.classList.add('ef-row-exit') | ||
| row.addEventListener('animationend', () => { | ||
| // Phase 2: smoothly collapse the space | ||
| row.style.height = row.offsetHeight + 'px' | ||
| row.style.overflow = 'hidden' | ||
| row.offsetHeight // force reflow | ||
| row.style.transition = 'height .15s ease-in, margin .15s ease-in' | ||
| row.style.height = '0' | ||
| row.style.marginBottom = '0' | ||
| row.addEventListener('transitionend', () => { | ||
| row.remove() | ||
| if (onComplete) onComplete() |
There was a problem hiding this comment.
animateGridRowOut() relies on animationend to start cleanup/removal. If the animation doesn’t run (e.g., reduced-motion settings, missing/blocked CSS due to CSP, or other style overrides), animationend may never fire and the row (and any dependent cleanup callback) will be stuck. Add a fallback (e.g., timeout, or detecting prefers-reduced-motion and skipping straight to the collapse/removal) so rows are always removed and onComplete is guaranteed to run.
| row.classList.add('ef-row-exit') | |
| row.addEventListener('animationend', () => { | |
| // Phase 2: smoothly collapse the space | |
| row.style.height = row.offsetHeight + 'px' | |
| row.style.overflow = 'hidden' | |
| row.offsetHeight // force reflow | |
| row.style.transition = 'height .15s ease-in, margin .15s ease-in' | |
| row.style.height = '0' | |
| row.style.marginBottom = '0' | |
| row.addEventListener('transitionend', () => { | |
| row.remove() | |
| if (onComplete) onComplete() | |
| // If user prefers reduced motion (or matchMedia is unavailable), skip animations. | |
| try { | |
| if (typeof window !== 'undefined' && | |
| typeof window.matchMedia === 'function' && | |
| window.matchMedia('(prefers-reduced-motion: reduce)').matches) { | |
| row.remove() | |
| if (onComplete) onComplete() | |
| return | |
| } | |
| } catch (e) { | |
| // If matchMedia throws for any reason, fall through to animated path with timeouts. | |
| } | |
| let cleanedUp = false | |
| const cleanup = () => { | |
| if (cleanedUp) return | |
| cleanedUp = true | |
| row.remove() | |
| if (onComplete) onComplete() | |
| } | |
| row.classList.add('ef-row-exit') | |
| // Fallback in case animationend never fires (no animation applied, CSS blocked, etc.) | |
| const animationFallbackTimeout = setTimeout(() => { | |
| // If the animation didn't start or finish, just clean up. | |
| cleanup() | |
| }, 500) | |
| row.addEventListener('animationend', () => { | |
| clearTimeout(animationFallbackTimeout) | |
| // Phase 2: smoothly collapse the space | |
| row.style.height = row.offsetHeight + 'px' | |
| row.style.overflow = 'hidden' | |
| // Force reflow so the browser picks up the starting height before transition. | |
| row.offsetHeight | |
| row.style.transition = 'height .15s ease-in, margin .15s ease-in' | |
| row.style.height = '0' | |
| row.style.marginBottom = '0' | |
| // Fallback in case transitionend never fires. | |
| const transitionFallbackTimeout = setTimeout(() => { | |
| cleanup() | |
| }, 250) | |
| row.addEventListener('transitionend', () => { | |
| clearTimeout(transitionFallbackTimeout) | |
| cleanup() |
| test('dynamic_rows_field hides add and remove buttons', function () { | ||
| createTestForm('grid_dynamic', [ | ||
| [ | ||
| 'handle' => 'num_passengers', | ||
| 'field' => [ | ||
| 'type' => 'integer', | ||
| 'display' => 'Number of Passengers', | ||
| ], | ||
| ], | ||
| [ | ||
| 'handle' => 'passengers', | ||
| 'field' => [ | ||
| 'type' => 'grid', | ||
| 'display' => 'Passengers', | ||
| 'dynamic_rows_field' => 'num_passengers', | ||
| 'fields' => [ | ||
| ['handle' => 'name', 'field' => ['type' => 'text', 'display' => 'Name']], | ||
| ], | ||
| ], | ||
| ], | ||
| ]); | ||
|
|
||
| $output = renderEasyFormTag('grid_dynamic'); | ||
|
|
||
| // is_fixed should be true, so no add/remove buttons | ||
| expect($output) | ||
| ->not->toContain("addGridRow('passengers')") | ||
| ->not->toContain("removeGridRow('passengers'") | ||
| ->not->toContain("canAddGridRow('passengers')") | ||
| ->not->toContain("canRemoveGridRow('passengers')"); | ||
| }); | ||
|
|
||
| test('dynamic_rows_field passes initDynamicGridRows to template', function () { | ||
| createTestForm('grid_dynamic_init', [ | ||
| [ | ||
| 'handle' => 'count', | ||
| 'field' => [ | ||
| 'type' => 'integer', | ||
| 'display' => 'Count', | ||
| ], | ||
| ], | ||
| [ | ||
| 'handle' => 'rows', | ||
| 'field' => [ | ||
| 'type' => 'grid', | ||
| 'display' => 'Rows', | ||
| 'dynamic_rows_field' => 'count', | ||
| 'fields' => [ | ||
| ['handle' => 'value', 'field' => ['type' => 'text', 'display' => 'Value']], | ||
| ], | ||
| ], | ||
| ], | ||
| ]); | ||
|
|
||
| $output = renderEasyFormTag('grid_dynamic_init'); | ||
|
|
||
| expect($output) | ||
| ->toContain("initDynamicGridRows('rows', 'count')"); | ||
| }); | ||
|
|
||
| test('dynamic_rows_field uses controlling field value in x-init', function () { | ||
| createTestForm('grid_dynamic_xinit', [ | ||
| [ | ||
| 'handle' => 'num_items', | ||
| 'field' => [ | ||
| 'type' => 'integer', | ||
| 'display' => 'Number of Items', | ||
| ], | ||
| ], | ||
| [ | ||
| 'handle' => 'items', | ||
| 'field' => [ | ||
| 'type' => 'grid', | ||
| 'display' => 'Items', | ||
| 'dynamic_rows_field' => 'num_items', | ||
| 'fields' => [ | ||
| ['handle' => 'name', 'field' => ['type' => 'text', 'display' => 'Name']], | ||
| ], | ||
| ], | ||
| ], | ||
| ]); | ||
|
|
||
| $output = renderEasyFormTag('grid_dynamic_xinit'); | ||
|
|
||
| expect($output) | ||
| ->toContain("parseInt(submitFields['num_items'])"); | ||
| }); | ||
|
|
||
| test('fixed_rows takes precedence over dynamic_rows_field', function () { | ||
| createTestForm('grid_fixed_wins', [ | ||
| [ | ||
| 'handle' => 'count', | ||
| 'field' => [ | ||
| 'type' => 'integer', | ||
| 'display' => 'Count', | ||
| ], | ||
| ], | ||
| [ | ||
| 'handle' => 'rows', | ||
| 'field' => [ | ||
| 'type' => 'grid', | ||
| 'display' => 'Rows', | ||
| 'fixed_rows' => 3, | ||
| 'dynamic_rows_field' => 'count', | ||
| 'fields' => [ | ||
| ['handle' => 'value', 'field' => ['type' => 'text', 'display' => 'Value']], | ||
| ], | ||
| ], | ||
| ], | ||
| ]); | ||
|
|
||
| $output = renderEasyFormTag('grid_fixed_wins'); | ||
|
|
||
| // fixed_rows is set, so dynamic behavior should not appear | ||
| expect($output) | ||
| ->toContain('|| 3') | ||
| ->not->toContain("initDynamicGridRows"); | ||
| }); | ||
|
|
||
| test('grid_rows tag parameter overrides dynamic_rows_field', function () { | ||
| createTestForm('grid_tag_wins', [ | ||
| [ | ||
| 'handle' => 'count', | ||
| 'field' => [ | ||
| 'type' => 'integer', | ||
| 'display' => 'Count', | ||
| ], | ||
| ], | ||
| [ | ||
| 'handle' => 'passengers', | ||
| 'field' => [ | ||
| 'type' => 'grid', | ||
| 'display' => 'Passengers', | ||
| 'dynamic_rows_field' => 'count', | ||
| 'fields' => [ | ||
| ['handle' => 'name', 'field' => ['type' => 'text', 'display' => 'Name']], | ||
| ], | ||
| ], | ||
| ], | ||
| ]); | ||
|
|
||
| $output = renderEasyFormTag('grid_tag_wins', [ | ||
| 'grid_rows' => ['passengers' => 5], | ||
| ]); | ||
|
|
||
| // Tag parameter should win: fixed at 5, no dynamic behavior | ||
| expect($output) | ||
| ->toContain('|| 5') | ||
| ->not->toContain("initDynamicGridRows") | ||
| ->not->toContain("addGridRow('passengers')"); | ||
| }); |
There was a problem hiding this comment.
The new tests assert template output strings, but the dynamic row feature’s most failure-prone cases aren’t covered (e.g., controlling field defined after the grid field, and controlling field values provided via prepopulated data / old input that should affect the initial row count). Adding coverage for at least one of these scenarios would help prevent regressions in the JS initialization/watch behavior.
| } else { | ||
| for (let i = currentCount - 1; i >= count; i--) { | ||
| field.grid_fields.forEach(f => { | ||
| delete this.submitFields[`${handle}.${i}.${f.handle}`] | ||
| }) | ||
| const row = container?.querySelector(`[data-grid-row="${i}"]`) | ||
| if (row) { | ||
| this.animateGridRowOut(row) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
When setGridRowCount() reduces the number of rows, it deletes state and removes DOM nodes but it never dispatches the grid-row-removed event (used by the parent formHandler to drop/shift validation errors). This can leave stale error entries for removed rows, causing error summaries/scrolling to reference non-existent fields. Consider dispatching grid-row-removed for each removed index (or adding a dedicated event to clear errors for indices >= new count).
| <style> | ||
| @keyframes ef-grid-row-in { from { opacity: 0; transform: translateY(-.5rem); } } | ||
| @keyframes ef-grid-row-out { to { opacity: 0; transform: translateY(-.5rem); } } | ||
| [data-grid-row].ef-row-enter { animation: ef-grid-row-in .2s ease-out; } | ||
| [data-grid-row].ef-row-exit { animation: ef-grid-row-out .15s ease-in forwards; pointer-events: none; } | ||
| </style> |
There was a problem hiding this comment.
Inline <style> in the grid field template will be emitted once per grid instance, which can bloat HTML output and can break sites with strict CSP (no inline styles). Consider moving these keyframes/class rules into the package CSS (e.g., the compiled easy-forms.css) and only toggling classes from JS in the template.
| let rowCount | ||
| if (field.dynamic_rows_field) { | ||
| const controlValue = parseInt(acc[field.dynamic_rows_field]) || 0 | ||
| rowCount = Math.max(controlValue, field.min_rows || 0) | ||
| if (field.max_rows) { | ||
| rowCount = Math.min(rowCount, field.max_rows) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Dynamic grid initialization reads the controlling field value from the partially-built acc object. This makes rowCount dependent on field order: if the grid field appears before the controlling field in the blueprint, acc[field.dynamic_rows_field] will be undefined and the grid will initialize with the wrong number of rows. A more robust approach is a two-pass initialization (initialize non-grid fields first, then grids) or to resolve the controlling field’s default from fields instead of acc.
| this.$watch( | ||
| () => this.submitFields[controlFieldHandle], | ||
| (newValue) => this.setGridRowCount(handle, newValue) | ||
| ) |
There was a problem hiding this comment.
initDynamicGridRows() registers a watcher but never applies the current controlling field value immediately. If the controlling field is prepopulated (or already set) before this watcher is registered, the callback won’t run and the grid can render with an out-of-sync row count until the user changes the control field. Call setGridRowCount(handle, this.submitFields[controlFieldHandle]) once during initialization (in addition to the watcher) to ensure correct initial state.
| ) | |
| ) | |
| // Ensure the grid is initialized with the current controlling field value | |
| this.setGridRowCount(handle, this.submitFields[controlFieldHandle]) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (field.dynamic_rows_field) { | ||
| // Look up from acc first (already processed), then fall back to the field's default | ||
| const controlValue = parseInt( | ||
| acc[field.dynamic_rows_field] | ||
| ?? fields.find(f => f.handle === field.dynamic_rows_field)?.default | ||
| ) || 0 |
There was a problem hiding this comment.
initializeFields() computes dynamic grid row count by calling fields.find(...) inside the reduce() loop. That makes initialization O(n^2) in the number of fields, which can become noticeable for larger forms. Consider precomputing a handle→field map (or handle→default map) once before the reduce and doing O(1) lookups instead.
| @keyframes ef-grid-row-in { from { opacity: 0; transform: translateY(-.5rem); } } | ||
| @keyframes ef-grid-row-out { to { opacity: 0; transform: translateY(-.5rem); } } | ||
| [data-grid-row].ef-row-enter { animation: ef-grid-row-in .2s ease-out; } | ||
| [data-grid-row].ef-row-exit { animation: ef-grid-row-out .15s ease-in forwards; pointer-events: none; } |
There was a problem hiding this comment.
Row enter/exit animations don’t fully respect prefers-reduced-motion: animateGridRowOut() skips animation when reduced motion is enabled, but the CSS-driven .ef-row-enter animation will still run. Consider adding a @media (prefers-reduced-motion: reduce) override to disable both .ef-row-enter/.ef-row-exit animations (and any transitions) for users who request reduced motion.
| [data-grid-row].ef-row-exit { animation: ef-grid-row-out .15s ease-in forwards; pointer-events: none; } | |
| [data-grid-row].ef-row-exit { animation: ef-grid-row-out .15s ease-in forwards; pointer-events: none; } | |
| @media (prefers-reduced-motion: reduce) { | |
| [data-grid-row].ef-row-enter, | |
| [data-grid-row].ef-row-exit { | |
| animation: none !important; | |
| transition: none !important; | |
| } | |
| } |
| public function index(): string | ||
| { | ||
| $form = $this->getForm(); | ||
| $blueprint = $form->blueprint(); | ||
|
|
||
| $gridRows = $this->params->get('grid_rows', []); | ||
| if (! empty($gridRows) && is_array($gridRows)) { | ||
| $this->setGridRowOverrides($gridRows); | ||
| } | ||
|
|
||
| $sectionsData = $this->processSections($blueprint); | ||
|
|
There was a problem hiding this comment.
This change removes support for the grid_rows tag parameter (previously used to override grid row counts at render time). If this tag parameter is part of the public API, this is a breaking change for existing templates. Consider adding a deprecation path (or at least documenting the removal / migration to dynamic_rows_field / fixed_rows).
No description provided.