ux icon indicating copy to clipboard operation
ux copied to clipboard

Add Collection component

Open dunglas opened this issue 3 years ago • 19 comments

Q A
Bug fix? no
New feature? yes
Tickets Fix #3, alternative to #90 and #397
License MIT

This alternative to #90 is adding support for dynamic collection in forms (adding and removing elements).

https://user-images.githubusercontent.com/57224/180029620-8bb52bab-7fb0-4cd4-89e4-b52c32b34dcf.mov

The two main benefits of this PR are:

  1. This pure JS implementation doesn't require custom form types. It is plug & play: you just have to install the bundle, it natively works with the default 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.
  2. The code natively handles any level of nested collections, which, as you can see in the code, is quite tricky.

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:

<div data-controller="symfony--ux-collection--collection">
    {{ form(form) }}
</div>

TODO:

  • [ ] Write more tests
  • [ ] Write docs

dunglas avatar Jul 18 '22 22:07 dunglas

I would not go with a plug & play integration. That will just make exist projects which already using an own data-prototype on other cases crash. I would go with an explicit ux collection type which also can be extended with other feature in the future. E.g. Copy & Paste, Sortable, ...

If you want to have a look at it I currently already implemented a Form Type here https://github.com/symfony/ux/pull/397.

alexander-schranz avatar Jul 18 '22 23:07 alexander-schranz

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 data-prototype is an attribute very specific to Symfony. The core framework should have used a prefix, for sure, but I'm not aware of any other popular project using this convention. We could make the attribute to look for configurable, but I'm not sure it is worth it.

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. Currently, this core feature is very hard to use because the JS included in the documentation as well as the existing NPM packages we recommend have bugs.

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.

dunglas avatar Jul 19 '22 05:07 dunglas

I understand your target of the pull request. But handling all [data-prototype] as a collection I would really avoid. I have in my current projects other form types working with prototypes which are not a collection.

If you want togo with a Plug & Play the easiet way would be go with a FormExtension adding the data-controller or if you want to keep your implementation go with data-collection-target="collection" to all CollectionType. Current implementation with parent.querySelector('[data-prototype]') hurts the usage of a component library like stimulus where selectors should always be limited scope like you did for the item selector.

But then we are in the second case not all CollectionType should be handled by this component. There are cases where I want to create or even have already created a custom stimulus component for a CollectionType. The current implementation would hurt this component as it tries to start on this collection a component which is already handled by itself. And would not allow me using this collection side by side with my other collection components.

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 Type allows us to add also seperate controller for "sortable" to make our entries sortable in the future.

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 disagree here that this is a good idea. As in some cases I want to use a Dropzone and in some oter cases I want to have FileType same is for the Collection. Forcing a specific JS component for a specific existing form type I would avoid.

alexander-schranz avatar Jul 19 '22 06:07 alexander-schranz

But handling all [data-prototype] as a collection

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 disagree here that this is a good idea. As in some cases I want to use a Dropzone and in some oter cases I want to have FileType same is for the Collection.

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 data-prototype things currently generated by core and recommend using a new UX form type instead, but IMHO (as this patch proves) it's not needed. With this approach, we'll be able to progressively improve the HTML generated by core (switching to <template> instead of data-prototype using a config flag, adding useful new ids, classes and attributes that will help writing custom JS etc).

Forcing a specific JS component for a specific existing form type I would avoid.

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 did go with https://github.com/symfony/ux/pull/397 to make the things extendable

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)?

dunglas avatar Jul 19 '22 09:07 dunglas

If you can, could you please some screenshots and/or short GIF showing this feature in action for complex forms? Thanks!

javiereguiluz avatar Jul 19 '22 10:07 javiereguiluz

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.

@dunglas Let me have a look at it. I will try to provide a prototype this week.

alexander-schranz avatar Jul 19 '22 11:07 alexander-schranz

In my use case (a two levels nested form), the code written in the docs can't help. That solution works just fine.

mano-lis avatar Jul 19 '22 12:07 mano-lis

Shouldn't we also ship the dist/controller.d.ts file alongside the compiled JS code ?

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.

dunglas avatar Jul 19 '22 16:07 dunglas

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.

stof avatar Jul 19 '22 16:07 stof

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:

Bildschirmfoto 2022-07-19 um 23 31 51

alexander-schranz avatar Jul 19 '22 21:07 alexander-schranz

@alexander-schranz thanks for sharing some screenshots.

Sorry for being blunt, but the autoincremental numeric labels (0, 1, etc.) always looked ugly to me and it's one of the things I hoped this new component was going to fix 🙏

javiereguiluz avatar Jul 20 '22 07:07 javiereguiluz

@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 label is not explicit set.

alexander-schranz avatar Jul 20 '22 08:07 alexander-schranz

@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.

stof avatar Jul 20 '22 08:07 stof

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.

dunglas avatar Jul 20 '22 15:07 dunglas

@javiereguiluz here is a recording:

https://user-images.githubusercontent.com/57224/180029620-8bb52bab-7fb0-4cd4-89e4-b52c32b34dcf.mov

dunglas avatar Jul 20 '22 16:07 dunglas

I'm not sure if it helps settle the debate around the data-prototype attribute but Sylius is using the same attribute to render a different prototype depending on a select element's value. Source JS: https://github.com/Sylius/Sylius/blob/204c60bc7faf9caff2378d4204680129a4379740/src/Sylius/Bundle/UiBundle/Resources/private/js/sylius-prototype-handler.js Example UI: image

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. Source JS: https://github.com/Sylius/Sylius/blob/204c60bc7faf9caff2378d4204680129a4379740/src/Sylius/Bundle/UiBundle/Resources/private/js/sylius-form-collection.js Example UI: image

vvasiloi avatar Jul 20 '22 23:07 vvasiloi

@vvasiloi thats also the same case for my projects, why I would avoid that selector. added comment here: https://github.com/symfony/ux/pull/398#discussion_r925859182

alexander-schranz avatar Jul 21 '22 09:07 alexander-schranz

Hi! I'd love to have this get pushed forward - we also have a competing PR at #400, which seems to address some issues in this PR. @dunglas WDYT about #400? Which PR should continue? This is a pretty popular feature.

Thanks!

weaverryan avatar Sep 22 '22 10:09 weaverryan

400 looks interesting.

I think we have a choice to make:

  • either we remove the HTML generation code needed for this feature from core and move it in UX
  • or we fix/improve the HTML generated by core (using <template> for instance) and we provide only the JS in UX (this is my preferred approach and we I try do do here as it's more future-proof in case new JS tools emerge etc)

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.

dunglas avatar Sep 22 '22 11:09 dunglas