Skip to content

[TwigBridge] Add row_attr to all form themes #33573

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

Merged
merged 1 commit into from
Nov 28, 2019

Conversation

fancyweb
Copy link
Contributor

@fancyweb fancyweb commented Sep 13, 2019

Q A
Branch? 4.3
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #33552
License MIT
Doc PR -

The rules I applied:

  • Always done on the first HTML tag of the row.
  • Current existing row attrs (class or style) are applied unless they are defined by the row_attr override. They can be removed if they are explicitly set to false.

Starting from:

<div class="form-group">

With row_attr: {foo: "bar"}:

<div foo="bar" class="form-group">

With row_attr: {class: "ccc"}:

<div class="ccc">

With row_attr: {foo: "bar", class: false}:

<div foo="bar">

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Sep 14, 2019
xabbuh added a commit that referenced this pull request Sep 25, 2019
This PR was merged into the 4.3 branch.

Discussion
----------

Add missing row_attr option to FormType

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix: #33682 - related issue #33573
| License       | MIT

The #33573 modified Symfony's form themes. But the [FormType](https://github.com/symfony/form/blob/master/Extension/Core/Type/FormType.php) don't allow the option `row_attr` so the OptionResolver throw an exception that the option is unknown.

This PR basically add the option and give it to the form view (like `label_attr` do)

Commits
-------

d711ea2 Add missing row_attr option to FormType
@xabbuh
Copy link
Member

xabbuh commented Sep 25, 2019

@fancyweb FYI, #33688 has been merged

@fancyweb fancyweb force-pushed the twig-bridge-row-attr branch from b3e9a65 to 310d6bb Compare September 26, 2019 15:47
@fancyweb
Copy link
Contributor Author

Here is what I did :

  1. Remove all defaults (cf Add missing row_attr option to FormType #33688)

  2. About [TwigBridge] Add row_attr to all form themes #33573 (comment) -> Always add current attr values (like what we do for the label attr class). That means end users can easily add attr values but not remove them. That's consistent with the current codebase + better for DX. If end users wants to remove default classes, they just have to override the whole block like currently. Creating dedicated blocks as I suggested is possible but requires more code and is probably not needed. Removing default classes makes no sense anyway (since it probably break the display).

@fancyweb fancyweb force-pushed the twig-bridge-row-attr branch 2 times, most recently from 9146363 to 6ac53d7 Compare September 26, 2019 16:38
@fancyweb fancyweb force-pushed the twig-bridge-row-attr branch from 6ac53d7 to 979aef5 Compare September 27, 2019 09:43
@loevgaard
Copy link
Contributor

What's the status with this PR? :)

@fancyweb fancyweb force-pushed the twig-bridge-row-attr branch from 94b04a8 to dfdcbb4 Compare November 21, 2019 13:35
@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

nicolas-grekas added a commit that referenced this pull request Nov 28, 2019
This PR was merged into the 4.3 branch.

Discussion
----------

[TwigBridge] Add row_attr to all form themes

| Q             | A
| ------------- | ---
| Branch?       | 4.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #33552
| License       | MIT
| Doc PR        | -

The rules I applied:
- Always done on the first HTML tag of the row.
- Current existing row attrs (`class` or `style`) are applied unless they are defined by the `row_attr` override. They can be removed if they are explicitly set to `false`.

Starting from:
```
<div class="form-group">
```

With `row_attr: {foo: "bar"}`:
```
<div foo="bar" class="form-group">
```

With `row_attr: {class: "ccc"}`:
```
<div class="ccc">
```

With `row_attr: {foo: "bar", class: false}`:
```
<div foo="bar">
```

Commits
-------

dfdcbb4 [TwigBridge] Add row_attr to all form themes
@nicolas-grekas nicolas-grekas merged commit dfdcbb4 into symfony:4.3 Nov 28, 2019
@fancyweb fancyweb deleted the twig-bridge-row-attr branch November 28, 2019 12:16
This was referenced Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants