Kreyu / data-table-bundle

Streamlines creation process of the data tables in Symfony applications. NOT PRODUCTION READY.
https://data-table-bundle.swroblewski.pl
MIT License
75 stars 14 forks source link

#88 Add the DropdownAction #110

Open alexandre-castelain opened 4 months ago

alexandre-castelain commented 4 months ago

Hey !

This is the first step to create the DropdownAction. Do not merge it as is.

On the theme side :

On the PHP side :

This is the way I want to add row actions :

->addRowAction('multipleActions', DropdownActionType::class, [
    'actions' => function (RowActionBuilderInterface $builder) {
        return [
            $builder->addRowAction('update', ButtonActionType::class, [
                'href' => function (Product $product) {
                    return $this->urlGenerator->generate('app_product_update', [
                        'id' => $product->getId(),
                    ]);
                },
            ])
        ];
    },
])

As you can see, I inject a RowActionBuilderInterface in the callback to add the actions. And the DataTableBuilderInterface extends it. I like it, because we could add a DropdownAction to a DropdownAction if we want it. It adds a lot of flexibility.

At this point, I'm not sure where I should resolve this callback. Neither if we should inject a DataTableBuilder or another class. What do you think about it ? Any advice ?

Have a good day ! Alexandre

Kreyu commented 4 months ago

Hey @alexandre-castelain.

What if we could completely drop the RowActionBuilderInterface, and just use the DataTableBuilderInterface? We can use the createAction, createRowAction and createBatchAction methods from the builder. The dropdown action should not be limited to the row actions alone.

The DropdownActionType would accept a actions option with allowed types ActionBuilderInterface[] ActionBuilderInterface[], or callable:

$builder
  ->addRowAction('multipleActions', DropdownActionType::class, [
      'actions' => [
          $builder->createRowAction('update', ButtonActionType::class, [
              'href' => function (Product $product) {
                  return $this->urlGenerator->generate('app_product_update', [
                      'id' => $product->getId(),
                  ]);
              },
          ])
      },
  ])
  ->addRowAction('multipleActionsCallback', DropdownActionType::class, [
      'actions' => function (mixed $data) { // $data is row data, since its a row action
          return [
              $builder->createRowAction('update', ButtonActionType::class, [
                  'href' => function (Product $product) {
                      return $this->urlGenerator->generate('app_product_update', [
                          'id' => $product->getId(),
                      ]);
                  },
              ])
          ];
      },
  ])
;

It would work the same with addAction (global action) and addBatchAction (batch action).

At this point, I'm not sure where I should resolve this callback. Neither if we should inject a DataTableBuilder or another class.

I think, directly in the DropdownActionType, something like that ("pseudo" code - not tested of course, just an idea ๐Ÿ˜„):

final class DropdownActionType extends AbstractActionType
{
    public function buildView(ActionView $view, ActionInterface $action, array $options): void
    {
        $children = [];

        $actions = $view->vars['actions'];

        // Resolve the callable if parent is column value (the action is row action)
        if ($view->parent instanceof ColumnValueView) {
            $actions = $actions($view->parent->value);
        }

        foreach ($actions as $child) {
             if ($child instanceof ActionBuilderInterface) {
                 $child = $child->getAction();
             }

             $children[] = $child;
        }

        $view->vars = array_replace($view->vars, [
            'children' => $children,
        ]);
    }
}

And in the Twig, we could iterate through the children variable and render each action in a dropdown. And this is the hardest part for me, as we are accepting actions of any type inside the dropdown. How to render the button action, for example? Or some "weird" custom type, with fancy rendering? Maybe we should either create a second type, for example, a DropdownItemActionType, and only accept those types as children, or just nest another DropdownActionType by itself.

alexandre-castelain commented 4 months ago

What if we could completely drop the RowActionBuilderInterface, and just use the DataTableBuilderInterface? We can use the createAction, createRowAction and createBatchAction methods from the builder. The dropdown action should not be limited to the row actions alone.

I worked only on the row action at this time, but doing as you propose, we could do this :

->addRowAction('multipleActions', DropdownActionType::class, [
      'actions' => [
          $builder->createBatchAction('update', ButtonActionType::class, [...]
      ]
  ])

Add a batchAction in a rowAction. This doesn't make any sense to me.

Using a callback, we would be sure the builder has only the correct method to use for the current action type. For a DX point of view, it's easier if there's less options available. Also, if someone (as I did) use the "addAction" instead of the "createAction", the action would be added not on the correct place. So we should consider this

Maybe we should either create a second type, for example, a DropdownItemActionType, and only accept those types as children, or just nest another DropdownActionType by itself.

Yeah, I like this idea.

Kreyu commented 4 months ago

Add a batchAction in a rowAction. This doesn't make any sense to me. Using a callback, we would be sure the builder has only the correct method to use for the current action type. For a DX point of view, it's easier if there's less options available.

I think you're right. That could be handy for autocompletion. I still don't like the idea of requiring a callback for that, but maybe that's just me ๐Ÿ˜„ Also, remember that this also would be correct:

$builder->addRowAction('multipleActions', DropdownActionType::class, [
      'actions' => [
          $builder
              ->createAction('update', ButtonActionType::class, [...])
              ->setContext(ActionContext::Row)
      ]
])

However, you can still define a child action with any action type. We can validate this in buildView method, but I think there's no way to help with that in IDE autocompletion.

Also, we have to validate the context of each action though, probably in the buildView as well. We could also change the context of each action to the same as the parent, that would be handy, but probably too confusing in some cases ๐Ÿ˜„

Also, if someone (as I did) use the "addAction" instead of the "createAction", the action would be added not on the correct place. So we should consider this

That is true, and this will throw an exception, because the option does not allow values of type DataTableBuilderInterface (which is returned from the fluent addAction()). I don't think we can improve the DX without creating a ton of small interfaces for the data table builder. I'm worried that over time, the builders will implement many different interfaces just for the sake of autocompletion, which in my opinion is not worth it ๐Ÿ˜ž

However, another idea - we can change the definitions of the actions for the key-value map, like so:

$builder->addRowAction('multipleActions', DropdownActionType::class, [
      'actions' => [
          'update' => ['label' => 'Update something'],
          'delete' => ['label' => 'Delete something'],
      ]
]);

and then, based on the context of the parent action (global, row, batch) create an action of correct type and context:

final class DropdownActionType extends AbstractActionType
{
    public function buildAction(ActionBuilderInterface $builder, array $options): void
    {
        // like in CollectionColumnType, we're saving the factory as attribute,
        // so we can access it in buildView() without unnecessary dependency injection
        // the symfony form component is also doing this in source in CollectionType
        $builder->setAttribute('action_factory', $builder->getActionFactory());
    }

    public function buildView(ActionView $view, ActionInterface $action, array $options): void
    {
        /** @var ActionFactoryInterface $actionFactory */
        $actionFactory = $action->getConfig()->getAttribute('action_factory');

        $children = [];

        $actions = $view->vars['actions'];

        // Resolve the callable if parent is column value (the action is row action)
        if ($view->parent instanceof ColumnValueView) {
            $actions = $actions($view->parent->value);
        }

        foreach ($actions as $name => $options) {
             $children[] = $actionFactory
                 ->createNamedBuilder($name, DropdownActionType::class, $options) // here we use the correct action type
                 ->setContext($action->getConfig()->getContext()) // here we use the correct context (global/row/batch)
                 ->getAction()
             ;
        }

        $view->vars = array_replace($view->vars, [
            'children' => $children,
        ]);
    }
}

I feel like we're so close to establish a solid API for the dropdown action, yet so far ๐Ÿ˜…

Kreyu commented 4 months ago

On the second thought, I think we are slightly... overthinking this. What if we simply create a DropdownActionType with items options, where each item contains predefined options? If you think about it, what do we need to specify each dropdown item:

This can be handled without touching current builder API, does not have any weird quirks related to action context (is it row? is it batch?) nor action type (dropdown item is dropdown item, simple as that).

final class DropdownActionType extends AbstractActionType
{
    public function buildView(ActionView $view, ActionInterface $action, array $options): void
    {
        // resolve items, add to $view->vars
    }

    public function configureOptions(OptionsResolver $resolver): void
    {
        $resolver
            ->setDefault('items', function (OptionsResolver $itemResolver): void {
                $itemResolver
                    ->setDefaults([
                        'url' => '#',
                        'method' => 'GET',
                        'attr' => [],
                    ])
                    ->setRequired('label')
                    ->setAllowedTypes('label', 'string')
                    ->setAllowedTypes('url', 'string')
                    ->setAllowedTypes('method', ['null', 'string'])
                    ->setAllowedTypes('attr', 'array')
                  ;
            })
        ;
    }
}

Now, I don't know if this method has any drawbacks, but what if we render each item as a form - so it can do any given method? https://jsfiddle.net/oravm9bf/5/

<div class="dropdown">
    <button class="btn btn-secondary dropdown-toggle" type="button" data-bs-toggle="dropdown" aria-expanded="false">
        Dropdown button
    </button>

    <ul class="dropdown-menu">
        {% for item in items %}
            <li>
                {# Note: form element method is either GET or POST, other methods belong to hidden _method input #} 
                <form action="{{ url }}" method="{{ method == 'GET' ? 'GET' : 'POST' }}">
                    <input type="hidden" name="_method" value="{{ method }}">
                    {# Here we should merge "attr" with type, class and value attributes and render it #}
                    {# using "attributes" block, but for the sake of simplicity of this example, I'm skipping it #}
                    <input type="submit" class="dropdown-item" value="{{ label }}" {{ block('attributes') }} />
                </form>
            </li>
        {% endfor %}
    </ul>
</div>

This is something I was thinking about to merge built-in FormActionType into ButtonActionType, to make it more simple and easier to use. Optionally, we can check if item method option equals "GET", and then, render it as a simple anchor:

<div class="dropdown">
    <button class="btn btn-secondary dropdown-toggle" type="button" data-bs-toggle="dropdown" aria-expanded="false">
        Dropdown button
    </button>

    <ul class="dropdown-menu">
        {% for item in items %}
            <li>
                {% if method == 'GET' %}
                    <a class="dropdown-item" href="{{ url }}" {{ block('attributes') }}>{{ label }}</a>
                {% else %}
                    <form action="{{ url }}" method="POST">
                        <input type="hidden" name="_method" value="{{ method }}">
                        <input type="submit" class="dropdown-item" value="{{ label }}" {{ block('attributes') }} />
                    </form>
                {% endif %}
            </li>
        {% endfor %}
    </ul>
</div>

What do you think? I think we can skip a lot of complexity, while also giving a lot of new functionality. Easier to use (API not changed, options easily validated), and easier to maintain. You can even handle each action with JavaScript, for example, with Stimulus, by simply adding an data-action attribute.

alexandre-castelain commented 4 months ago

Hey,

I'll try to focus on your second proposal, because it looks promising.

On the second thought, I think we are slightly... overthinking this. What if we simply create a DropdownActionType with items options, where each item contains predefined options? If you think about it, what do we need to specify each dropdown item:

  • a label, obviously, to display in the dropdown;
  • an url to execute the action;
  • a method, for actions using something other than GET, e.g. a DELETE for entity removal;
  • an array of HTML attributes for the dropdown item;

Clearly, this approach seems much simpler. On the other hand, I also find that we lose the reuse of other actions.

For example:

I think if we go with this "simplified" approach, we might quickly find ourselves limited in more complex cases.

However, I think that by slightly modifying your proposal, we could achieve something interesting.

For example:

$builder->addRowAction('multipleActions', DropdownActionType::class, [
    'actions' => [
        'update' => ['type' => LinkActionType::class, 'options' => ['label' => 'Update something']],
        'delete' => ['type' => FormActionType::class, 'options' => ['label' => 'Delete something']],
    ]
]);

Iโ€™m not sure if I am entirely convinced by this proposal.

Advantages:

Disadvantages:

Another proposal:

$builder->addRowAction('multipleActions', DropdownActionType::class, [
    'actions' => [
        new DropdownActionItem('update', LinkActionType::class, ['label' => 'Update something']),
        new DropdownActionItem('delete', FormActionType::class, ['label' => 'Delete something']),
    ]
]);

Advantages:

Disadvantages:

It would be ideal if 'actions' could accept either an array or a callback (yeah I know, another callback !) to retrieve the values of the current row:

$builder->addRowAction('multipleActions', DropdownActionType::class, [
    'actions' => function (Product $product) {
        return [
            new DropdownActionItem('update', LinkActionType::class, [
                'label' => 'Update something', 
                'url' => $this->router->generate('app_update_product', ['id' => $product->getId()])
            ]),
            new DropdownActionItem('delete', FormActionType::class, ['label' => 'Delete something']),
        ];
    }
]);
Kreyu commented 4 months ago

Clearly, this approach seems much simpler. On the other hand, I also find that we lose the reuse of other actions.

Okay, let's ignore the options and builder API for a moment. Reusing other action types seems cool, and I would be all-in on that idea, but how do we render them?

If I use a dropdown action, I would expect them to render as a classic dropdown list:

image

If we want to reuse other actions, this is impossible to render in that way, because each action type HTML may or may not be suitable for a dropdown (take into account custom action types and themes):

image

We cannot modify each child action HTML on render to make it suitable for a dropdown, as each type and theme can render those differently. Because we are not limiting how each action type can be configured and rendered (total freedom!), this also prevents us from reusing them in a dropdown without creating a monstrosity of a HTML :grimacing:

In fact, I would say, that we:

alexandre-castelain commented 4 months ago

Maybe a naive question, but: wouldn't it be possible to create a specific "sub" block?

For the ActionLinkType, for example:

This way, depending on the theme, we can "redesign" the way the HTML is displayed in the Dropdown without touching the default HTML.

I am not used to working with Twig themes like this, so I might be completely off track!

Kreyu commented 4 months ago

Maybe a naive question, but: wouldn't it be possible to create a specific "sub" block?

Yeah, we could do it in the same way the column is split into two parts - a "header" and a "value".

Currently, action Twig blocks are suffixed with _value, which I'm not fond of. Maybe we should seize the opportunity and refactor it into _control, and for dropdowns, something like _dropdown_control. Then, add Twig function named data_table_dropdown_action, similar to data_table_action, that will render the proper block (with fallback to parent type block).

Also, we would have to add something like buildDropdownView() method to the ActionTypeInterface, that lets you build a separate ActionView for the dropdown. This, in my opinion, will be inconvenient, because most action types will require duplicate logic inside of buildView() and buildDropdownView(), with resolving the options and passing them to the view. Then, passing options specific for a specific view will be hard. For example, if we pass HTML attributes using the attr option, do they belong to the "regular" action view, or the dropdown one? Wouldn't that require adding separate option prefixed with dropdown_? It seems like it doubles the complexity of an action configuration ๐Ÿ˜ž. Maybe this is fine, but I wonder if this is not too complicated for a simple dropdown.

I know it would be possible, but I really don't want to increase complexity like that just for the sake of a single dropdown action type.

On the other hand... shouldn't this be a theme-specific feature? For example, we could add a theme that groups actions into one dropdown, and renders them properly (the theme will simply contain proper HTML for each action type). This way, we can use this theme in data tables that require this feature. This also means, that we would no longer mix and match inline actions with dropdown actions, but I think that's how its done in, for example, a EasyAdmin - actions are either inline or not. For now, we could do something like bootstrap_5_dropdown_actions.html.twig, that, when imported after the Bootstrap 5 theme, it would render actions as a dropdown, instead of a default inline method. That method would be really simple (just a one twig file), totally optional, and, I think, a good compromise between complexity and functionality.