Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GridBuilder] Apply transition action #305

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

loic425
Copy link
Member

@loic425 loic425 commented Jun 2, 2023

An helper for this kind of feature:
https://github.com/Sylius/Sylius/blob/1.13/src/Sylius/Bundle/AdminBundle/Resources/config/grids/product_review.yml#L56

I choose not to add "graph" on the action constructor and let the developer add it on the "options" side.
The reason is that is it's not mandatory and Symfony workflow & Winzou state machine have 2 default values (default for Winzou and null for Symfony workflow).

The current template which is a Winzou specific one.
I do not remember if I've changed sth on that template on my Workflow PoC.

<!-- @SyliusUi/Grid/Action/applyTransition.html.twig -->

{% set labeled = options.labeled is defined ? options.labeled : true %}

{% if sm_can(data, options.transition, options.graph) %}
    <form action="{{ path(options.link.route, options.link.parameters) }}" method="post">
        <input type="hidden" name="_csrf_token" value="{{ csrf_token(data.id) }}">
        <input type="hidden" name="_method" value="PUT">
        <button class="ui loadable {{ options.class|default }} {% if labeled %}labeled{% endif %} icon button" type="submit" data-js-disable=".sylius-grid-table-wrapper button, .sylius-grid-table-wrapper a">
            <i class="{{ action.icon }} icon"></i>
            {% if labeled %}{{ action.label|trans }}{% endif %}
        </button>
    </form>
{% endif %}

I do not like the way "setOptions" has been done (https://github.com/Sylius/SyliusGridBundle/blob/1.13/src/Bundle/Builder/Action/Action.php#L69). I think it's very confusing that it removes the existing values.
I think we should merge the Options for a future version and add a removeOption method instead to remove a specific option.

@loic425 loic425 force-pushed the feature/apply-transition-action branch from 058630c to 67f2ef8 Compare June 6, 2023 13:22
@loic425 loic425 force-pushed the feature/apply-transition-action branch from 67f2ef8 to 08148e0 Compare June 6, 2023 13:30

final class ApplyTransitionAction
{
public static function create(string $name, string $route, array $routeParameters = [], array $options = []): ActionInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still add transition and graph fields, as they would be usually used (looking at Sylius example)

Suggested change
public static function create(string $name, string $route, array $routeParameters = [], array $options = []): ActionInterface
public static function create(string $name, string $route, array $routeParameters = [], string $graph = 'default', ?string $transition = null, array $options = []): ActionInterface

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't work with symfony workflow, cause default value is null

@lchrusciel lchrusciel merged commit 4d7b863 into Sylius:1.13 Jun 8, 2023
@lchrusciel
Copy link
Member

Thank you, @loic425!

@loic425 loic425 deleted the feature/apply-transition-action branch June 14, 2023 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants