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

[Form][TwigBridge] Add row_attr to form theme #30320

Merged
merged 1 commit into from Mar 31, 2019

Conversation

Projects
None yet
9 participants
@alexander-schranz
Copy link
Contributor

commented Feb 20, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? waiting for confirmation to implement
Fixed tickets
License MIT
Doc PR symfony/symfony-docs#...

Currently you need to copy the whole form_row block if you just want to add a class to the form_row div. Instead we could introduce a row_attr which can then simple be set the following in the theme e.g.:

{% block form_row %}
    {% set row_attr = { class: 'form__field' } %}
    {{ parent() }}
{% endblock %}

or in php:

$builder->add('test', TextType::class, ['row_attr' => ['class' => 'form__field']]);
@OskarStark

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

I like the idea 👍

@nicolas-grekas nicolas-grekas added this to the next milestone Feb 23, 2019

@nicolas-grekas nicolas-grekas changed the title Add row_attr to form theme [Form][TwigBridge] Add row_attr to form theme Feb 23, 2019

@alexander-schranz alexander-schranz force-pushed the alexander-schranz:patch-2 branch from 6460b8a to 2d59e26 Feb 26, 2019

@xabbuh

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

I am not sure if this is a common enough use case or if it's enough to let users add this in their own form type extensions.

@alexander-schranz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

@xabbuh it would make it easer to create reusable themes. Currently you need to check if the installed symfony version has a form_help block or not.

Another fix to this issue could be adding a new block to the form_div_layout:

{% block form_row %}
    <div>
          {{ block('form_row_inner') }}
    </div>
{% endblock %}


{% block form_row_inner %}
    ....
{% endblock %}
@HeahDude

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I would not be in favor of adding yet a new option for the view level unless we can workaround another way.

I think we could just change that line https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig#L324 to set it only when it's not done before, that would allow you to reset only that value in your block. WDYT?

@stof

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I have an equivalent of this option (I named it wrapper_attr, but row_attr makes more sense as it goes on the div generated by form_row) in the Incenteev codebase since years.
Adding a type extension for that is not the painful part. Overriding all the *_row blocks in the theme to add support for the option is.

I vote 👍 for adding this in core.

@HeahDude

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I don't get why this should be an option? you just need to pass it as context in form_row calls if we allow it by changing the line I link, no?

{{ form(some_form, { widget_attr: ... }) }}

?

@alexander-schranz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@HeahDude but widget_attr is totally different what we want to change. We want to add attributes to the div of the form_row not the widget.

@HeahDude

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

Yes but we can do both if we add raw_attr there. That would make sense to have all levels configurable the same way, with the template context. Options are overhead also in the submission process.

@alexander-schranz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 11, 2019

@HeahDude now I get you. For me ok just add it to the theme and not to the FormType.

What do the others think? /cc @stof @OskarStark

@xabbuh

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

@HeahDude's suggestion sounds better to me than adding another option for this.

@HeahDude

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

To add an argument on this, ppl wanting an option can still rely on form type extension, and define their own targets which may not be FormType.

@fabpot

fabpot approved these changes Mar 31, 2019

@fabpot fabpot force-pushed the alexander-schranz:patch-2 branch from 2e70a70 to 7ab1b00 Mar 31, 2019

@fabpot

This comment has been minimized.

Copy link
Member

commented Mar 31, 2019

Thank you @alexander-schranz.

@fabpot fabpot merged commit 7ab1b00 into symfony:master Mar 31, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 31, 2019

feature #30320 [Form][TwigBridge] Add row_attr to form theme (alexand…
…er-schranz)

This PR was squashed before being merged into the 4.3-dev branch (closes #30320).

Discussion
----------

[Form][TwigBridge] Add row_attr to form theme

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | waiting for confirmation to implement
| Fixed tickets |
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Currently you need to copy the whole form_row block if you just want to add a class to the form_row div. Instead we could introduce a `row_attr` which can then simple be set the following in the theme e.g.:

```twig
{% block form_row %}
    {% set row_attr = { class: 'form__field' } %}
    {{ parent() }}
{% endblock %}
```

or in php:

```
$builder->add('test', TextType::class, ['row_attr' => ['class' => 'form__field']]);
```

Commits
-------

7ab1b00 [Form][TwigBridge] Add row_attr to form theme

@alexander-schranz alexander-schranz deleted the alexander-schranz:patch-2 branch Mar 31, 2019

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@tehplague

This comment has been minimized.

Copy link

commented May 10, 2019

Could you add this to the bootstrap form themes, too? Currently only the form_div_layout features the row_attr argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.