Add Collection component #398
Conversation
| I would not go with a plug & play integration. That will just make exist projects which already using an own If you want to have a look at it I currently already implemented a Form Type here #397. |
| Why would it crash? The code I wrote should be robust enough to not crash and is already configurable: you can use data attributes to customize the CSS selectors to use and the buttons to add. My implementation should work easily with any form theme, including custom ones. Also, AFAIK It's also totally possible to extend it to add extra features such as sorting. Dynamic collection is a feature that is already supported by the core framework. IMO Symfony should be as consistent and as easy to use as possible. Creating this kind of form is currently very hard, even for senior developers, and it should not. We also must not require to change the PHP code to use Symfony UX. Has much as possible, Symfony UX should provide JS code compatible with the features and especially the HTML generated by the core framework. |
| I understand your target of the pull request. But handling all If you want togo with a But then we are in the second case not all I did go with #397 to make the things extendable, even have the possibility iwth FormExtension to add new buttons for adding "Copy & Paste" functionality. Have the collection itself as controller and as own
I disagree here that this is a good idea. As in some cases I want to use a |
This isn't what the code does, it only targets the children of the HTML element using the controller (and it's not mandatory to use it if you want a custom behavior for a given form): https://github.com/symfony/ux/pull/398/files#diff-0c84303846c77e8d3377b23960aed4a1dfc3db0bb6206c7435ef6fe890a7fa1bR11 That being said, I totally agree that this controller must be compatible other UX components such as Dropzone etc, I'll update the code to support this!
I think we haven't the same use cases in mind. I crafted this patch because a junior developper from our company had to create a "simple" multi-level nested form... He read the official documentation entry, didn't managed to get his code working... and asked for help. I took a look and I figured out that it was almost impossible to use this feature. My main goal here is to have something that just works for simple cases (prototypes, MVP etc), that can be easily documented, and that doesn't require any code change. If we include a feature in core such as dynamic collection, it must not be half-finished, we most provide everything to get it working and it's currently not the case. This patch fixes this bad situation (it's a common use case, and probably hurts many newcomers). Also, in term of DX, a user should be able to have a working dynamic form without having to write custom PHP or Twig code. Another option would be to deprecate all the
You're not forced to that. You can use this controller on a per-form basis, use the existing (unfortunately buggy) jQuery and pure-JS packages or write your own JS. I really don't understand what's the issue here.
I'm pretty sure that we can merge both approaches and have something both plug & play and fully customizable. Would you be OK to try to merge both patches (cc @stakovicz)? |
| If you can, could you please some screenshots and/or short GIF showing this feature in action for complex forms? Thanks! |
| // If no template is provided, create a raw HTML button | ||
| const button = document.createElement('button') as HTMLButtonElement; | ||
| button.type = 'button'; | ||
| button.textContent = buttonType === ButtonType.Add ? 'Add' : 'Delete'; |
There was a problem hiding this comment.
this should support a way to customize the labels. The common use case is to translate them.
There was a problem hiding this comment.
This is supported using custom templates. Isn't it enough?
| const collectionNamePattern = collectionEl.id.replace(/_/g, '(?:_|\\[|]\\[)'); | ||
| | ||
| const prototype = (collectionEl.dataset.prototype as string) // We're sure that dataset.prototype exists, because of the CSS selector used in connect() | ||
| .replace('__name__label__', currentIndex) |
There was a problem hiding this comment.
The __name__ part is configuration in CollectionType (which is necessary to support nested collection types, as you should then use different prototype names for them))
There was a problem hiding this comment.
Actually my code supports nested collections even when not using another placeholder, but I'll add support for this option!
There was a problem hiding this comment.
No it does not. The prototype of the outer collection actually contains the attribute storing the prototype of the inner collection (as it contains the whole inner collection). If you use the same placeholder for both, the replacement done here will replace both placeholders inside the inner prototype, breaking the inner collection.
There was a problem hiding this comment.
Try doing a nested collection where you add multiple items in the inner collection of an item added in the outer collection, and then look at the submitted data in the Request (you need to inspect the request itself, not the data decoded in $_POST as duplicate keys would be lost during the conversion to a PHP array)
There was a problem hiding this comment.
the replacement done here will replace both placeholders inside the inner prototype
I didn't handle the label yet, but for __name__, the regex only replaces the first occurrence, which should be enough to fix this issue. I'll double-check.
There was a problem hiding this comment.
replacing only the first occurrence is also broken. __name__ will appear multiple time in the prototype when the entry type is a compound type (as the name of each child form will have the placeholder in its name.
There was a problem hiding this comment.
The first occurence in every string: https://github.com/symfony/ux/pull/398/files#diff-0c84303846c77e8d3377b23960aed4a1dfc3db0bb6206c7435ef6fe890a7fa1bR49
Unless I'm missing something, this works with the current example I provided for test purpose.
There was a problem hiding this comment.
@dunglas AFAICT, this would replace both placecholders in name="root[outer_collection][__name__][inner_collection][__name][field_1]"
And trying to do clever tricks based on the quote delimiting attributes (to replace the first occurrence in each attribute) is a bad idea: in the prototype attribute of the nested collection, those would be HTML entities and not actual quotes.
Note that you need to add multiple items in each collection to notice the issue (if you only add one outer item with a single inner item, you won't notice whether the [0] come from the replacements done by the outer or inner item)
| }; | ||
| | ||
| /* eslint-disable no-undef */ | ||
| describe('CollectionController', () => { |
There was a problem hiding this comment.
is there any way to make the tests cover the interactions (add and delete) ? I'm fearing that they might be broken right now due to adding the listeners on the DocumentFragment.
There was a problem hiding this comment.
Sure, I planned to add a Panther test.
There was a problem hiding this comment.
I don't think we need panther here if we use the testing library. I currently could not get your branch running. But something into this direction should work to test add and remove:
/* * This file is part of the Symfony package. * * (c) Fabien Potencier <fabien@symfony.com> * * For the full copyright and license information, please view the LICENSE * file that was distributed with this source code. */ 'use strict'; import { Application } from '@hotwired/stimulus'; import {fireEvent, getByTestId, getByText, waitFor} from '@testing-library/dom'; import { clearDOM, mountDOM } from '@symfony/stimulus-testing'; import CollectionController from '../src/controller'; const startStimulus = () => { const application = Application.start(); application.register('symfony--ux-collection--collection', CollectionController); }; const emptyCollection = '<div data-testid="collection" data-controller="symfony--ux-collection--collection"></div>'; const simpleCollection = '' + ' <template id="addButton">' + ' <button type="button" class="add-button">Add button</button>' + ' </template>' + '' + ' <template id="deleteButton">' + ' <button type="button" class="delete-button">Delete button</button>' + ' </template>' + '' + '<form name="game" method="post" data-controller="symfony--ux-collection--collection" data-add-button="addButton" data-delete-button="deleteButton">' + ' <div id="game">' + ' <div>' + ' <label class="required">Teams</label>' + ' <div id="game_teams" data-prototype="<div class="team-entry"><label class="required">__name__label__</label><div id="game_teams___name__"><div><label for="game_teams___name___name" class="required">Name</label><input type="text" id="game_teams___name___name" name="game[teams][__name__][name]" required="required" data-controller="test" /></div></div></div>">' + ' </div>' + ' </div>' + ' <div>' + ' <button type="submit" id="game_submit" name="game[submit]">Submit</button>' + ' </div>' + '</form>' const nestedCollection = '' + ' <template id="addButton">' + ' <button type="button" class="add-button">Add button</button>' + ' </template>' + '' + ' <template id="deleteButton">' + ' <button type="button" class="delete-button">Delete button</button>' + ' </template>' + '' + '<form name="game" method="post" data-controller="symfony--ux-collection--collection" data-add-button="addButton" data-delete-button="deleteButton">' + ' <div id="game">' + ' <div>' + ' <label class="required">Teams</label>' + ' <div id="game_teams" data-prototype="<div><label class="required">__name__label__</label><div id="game_teams___name__"><div><label for="game_teams___name___name" class="required">Name</label><input type="text" id="game_teams___name___name" name="game[teams][__name__][name]" required="required" data-controller="test" /></div><div><label class="required">Players</label><div id="game_teams___name___players" data-prototype="&lt;div&gt;&lt;label class=&quot;required&quot;&gt;__name__label__&lt;/label&gt;&lt;div id=&quot;game_teams___name___players___name__&quot;&gt;&lt;div&gt;&lt;label for=&quot;game_teams___name___players___name___firstName&quot; class=&quot;required&quot;&gt;First name&lt;/label&gt;&lt;input type=&quot;text&quot; id=&quot;game_teams___name___players___name___firstName&quot; name=&quot;game[teams][__name__][players][__name__][firstName]&quot; required=&quot;required&quot; /&gt;&lt;/div&gt;&lt;div&gt;&lt;label for=&quot;game_teams___name___players___name___lastName&quot; class=&quot;required&quot;&gt;Last name&lt;/label&gt;&lt;input type=&quot;text&quot; id=&quot;game_teams___name___players___name___lastName&quot; name=&quot;game[teams][__name__][players][__name__][lastName]&quot; required=&quot;required&quot; /&gt;&lt;/div&gt;&lt;/div&gt;&lt;/div&gt;"></div></div></div></div>">' + ' </div>' + ' </div>' + ' <div>' + ' <button type="submit" id="game_submit" name="game[submit]">Submit</button>' + ' </div>' + '</form>' /* eslint-disable no-undef */ describe('CollectionController', () => { startStimulus(); afterEach(() => { clearDOM(); }); it('connects', async () => { const container = mountDOM(emptyCollection); // smoke test expect(getByTestId(container, 'collection')).toHaveAttribute('data-controller', 'symfony--ux-collection--collection'); }); it('add a new collection', async () => { const container = mountDOM(simpleCollection); await waitFor(() => getByText(container, 'Add button')); fireEvent( getByText(container, 'Add button'), new MouseEvent('click', { bubbles: true, cancelable: true, }), ) expect(container.querySelectorAll('.team-entry').length).toBe(1); }); it('remove a collection', async () => { const container = mountDOM(simpleCollection); await waitFor(() => getByText(container, 'Add button')); fireEvent( getByText(container, 'Add button'), new MouseEvent('click', { bubbles: true, cancelable: true, }), ) expect(container.querySelectorAll('.team-entry').length).toBe(1); await waitFor(() => getByText(container, 'Delete button')); fireEvent( getByText(container, 'Delete button'), new MouseEvent('click', { bubbles: true, cancelable: true, }), ) expect(container.querySelectorAll('.team-entry').length).toBe(0); }); });There was a problem hiding this comment.
@dunglas could get it running and updated the code snippet. Currently only test for simple add and remove and no tests yet for the nested collection but already created the required html for it.
There was a problem hiding this comment.
IMHO it's better to create an E2E test here because we want to be sure that the HTML generated by the form component is still compatible with the JS code.
There was a problem hiding this comment.
The Panther tests would still be worthy as they would ensure that the submitted data is the one expected by the backend code.
| $this->teams[] = $team; | ||
| $this->teams = array_values($this->teams); | ||
| | ||
| dump($this->teams); |
| disableDeleteButton?: string; | ||
| } | ||
| | ||
| enum ButtonType { |
There was a problem hiding this comment.
This should be a const enum instead, to inline the add and remove string in the emitted JS code instead of emitting an enum object.
There was a problem hiding this comment.
I changed the code and using const enum isn't possible anymore (IIRC, const enum causes various issues anyway).
There was a problem hiding this comment.
Another alternative is to use a tagged union 'add' | 'remove', which will then produce more efficient JS code.
@dunglas Let me have a look at it. I will try to provide a prototype this week. |
| In my use case (a two levels nested form), the code written in the docs can't help. That solution works just fine. |
Declarations are entirely disabled for the whole project. I don't know why (cc @tgalopin) but if changed, this probably should be done globally in another PR. |
I was not aware of that. But this is indeed a separate discussion then. |
| I did on top of the different discussion (#90, #397, #398) create a form type extension based implementation here: #400. There are still things todo but maybe we can discuss in which direction a Collection UX component should go. For easier testing I also added some kind of form theme to the example:
|
| @alexander-schranz thanks for sharing some screenshots. Sorry for being blunt, but the autoincremental numeric labels ( |
| @javiereguiluz i normally disabled them via: $builder->add('slots', CollectionType::class, [ 'entry_options' => [ 'label' => false, ], ]);but didn't want to change the default behaviour. But a form extension could do that automatically if the |
| @javiereguiluz if you want something more human, this generally involves configuring the label in the options. I don't see how the JS code could automatically generate good-looking labels. |
| I added an example confirming that nested dynamic controllers are well supported, e.g if you use Dropzone in a form type that is added dynamically when clicking on the "add" button, the Stimulus correctly detects the added element and connects the controller automatically. |
| @javiereguiluz here is a recording: symfony-collection.mov |
| } | ||
| | ||
| connectCollection(parent: HTMLElement) { | ||
| parent.querySelectorAll('[data-prototype]').forEach((el) => { |
There was a problem hiding this comment.
I still want to add here a comment that this the most questionable line in my point of view.
In #400 I implemented the JS the way I think it should be implemented that every collection itself has its collection_controller and not that one controller controls all collections.
So we even don't need to wrap the whole form with a data-controller.
Why I would avoid the data-prototype selector here is that for example I have inside my current forms other Form Types using [data-prototye] which are even inside collection types, this component currently would identify them also as "collection" which they are not they just use the data-prototype for different usecases.
Via a form extension like in #400 we can add a better way by adding data-controller to all collection which would solve this problem and even allows to create collection type with a custom stimulus controller like in #90 or #397 also allows extending the current implementaton via events.
There was a problem hiding this comment.
I'll make this selector configurable and split this controller in two (the main one in case you want all collections to be automatically handled, and a nested one). WDYT?
There was a problem hiding this comment.
It still is hard to find here a correct selector as the main problems is that the component is registered on the form tag and handles multiple instances of collections.
As I did use component libraries over the last years which work very simular to stimulus I would really not recommend creating components like this which handles multiple instances. Instead I would make sure to have a collection_controller which handles the a single collection, which should make also the code simpler.
What problem do you see that the JS component handles just a single collection and adding a data-controller via form extension to the collection row like in #400.
| addAddButton(collectionEl: HTMLElement) { | ||
| const addButton = this.createButton(collectionEl, ButtonType.Add); | ||
| addButton.onclick = (e) => { | ||
| e.preventDefault(); | ||
| this.addItem(collectionEl); | ||
| }; | ||
| collectionEl.appendChild(addButton); | ||
| } | ||
| | ||
| addDeleteButton(collectionEl: HTMLElement, itemEl: HTMLElement) { | ||
| const deleteButton = this.createButton(collectionEl, ButtonType.Delete); | ||
| deleteButton.onclick = (e) => { | ||
| e.preventDefault(); | ||
| itemEl.remove(); | ||
| }; | ||
| itemEl.appendChild(deleteButton); | ||
| } |
There was a problem hiding this comment.
In #90 where was discussed that it would be nice that the buttons are rendered by the symfony form theme. That make it special for design systems easier so the can directly use there already designed button_widget and bootstrap and other themes already ships with a already good looking button. currently buttons are here if not explicit a template is defined rendered via this js component. in #400 I used the twig extension to add the buttons already to the twig and to the prototype, which did make also the JS a lot smaller which I think is great from maintainance point of view.
There was a problem hiding this comment.
My long-term goal is to totally remove data-prototype from core, generate <template> elements instead (which are better suited for this use case), and provide default buttons directly in the core form themes (as <template> elements too). However, during the deprecation process, we'll need to support both this new approach and data-prototype (for the "plug & play" DX) until Symfony 7.
I'll open the PR on core in the next few days.
There was a problem hiding this comment.
Agree still that template are better suited for the data-prototype. Still I recommed to have the buttons rendered already in twig because adding them via JS will trigger else a Layout shift and google and lighthouse hate layout shifts.
There was a problem hiding this comment.
Sure, that's the plan (and will be the default in Symfony 7!)
| @@ -0,0 +1,19 @@ | |||
| Copyright (c) 2021 Kévin Dunglas | |||
| I'm not sure if it helps settle the debate around the There's also a custom implementation of form collections that also supports dynamic prototypes based on select inputs just like the one previously mentioned but inside a collection. |
| if (!buttonTemplate) throw new Error(`template with ID "${buttonTemplate}" not found`); | ||
| | ||
| const fragment = buttonTemplate.content.cloneNode(true) as DocumentFragment; | ||
| if (1 !== fragment.children.length) |
There was a problem hiding this comment.
this could use fragment.childElementCount
| | ||
| const fragment = buttonTemplate.content.cloneNode(true) as DocumentFragment; | ||
| if (1 !== fragment.children.length) | ||
| throw new Error('template with ID "${buttonTemplateID}" must have exactly one child'); |
There was a problem hiding this comment.
this needs to use backticks, not single quotes. Otherwise the interpolation won't work.
| @vvasiloi thats also the same case for my projects, why I would avoid that selector. added comment here: #398 (comment) |
| 400 looks interesting. I think we have a choice to make:
What I don't like with the current state of 400 is that it overlaps with (partially broken) features that we already have in core. But I would be ok with deprecating these features from core and moving them to UX too. |
9 participants
This alternative to #90 is adding support for dynamic collection in forms (adding and removing elements).
symfony-collection.mov
The two main benefits of this PR are:
CollectionType. It works with the default theme, the Tailwind theme, and the Bootstrap theme. I haven't tested with the Foundation theme but it should be compatible too.The Stimulus controller provides several data attributes that can be used to customize the buttons when pointing to a
<template>tag. If this patch is accepted, I'll also provide some patches to the Form component to automatically leverage this feature to not generate the add and remove buttons when not needed according to the form options. We could also provide better default buttons directly in form themes for Tailwind, Bootstrap, and Foundation.Usage:
TODO: