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

[TwigBridge] Apply some changes to support Bootstrap4-stable #26167

Merged
merged 2 commits into from
Feb 15, 2018

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Feb 13, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25655 #25635 #24435
License MIT
Doc PR -

Follow up of #25715, see discussion there.

This fixes the following errors:

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Feb 13, 2018

@Nyholm I gave you access to my fork so you can finish this PR if you don't mind (tests esp. are broken for now.)

{%- set prepend = not (money_pattern starts with '{{') -%}
{%- set append = not (money_pattern ends with '}}') -%}
{%- if prepend or append -%}
<div class="input-group{{ group_class|default('') }}">
Copy link
Member

Choose a reason for hiding this comment

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

is group_class defined here ?

Copy link
Member

Choose a reason for hiding this comment

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

Not in this block no. The abstract form has the same code: https://github.com/symfony/symfony/blob/v4.0.4/src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_base_layout.html.twig#L14

I guess it was just copied and added some modifications.

@@ -53,7 +53,7 @@ public function testLabelOnForm()

$this->assertMatchesXpath($html,
'/legend
[@class="col-form-label col-sm-2 col-form-legend required"]
[@class="col-form-label col-sm-2 col-form-label required"]
Copy link
Member

Choose a reason for hiding this comment

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

Here is the col-form-label added twice. Do we bother?

Copy link
Member

Choose a reason for hiding this comment

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

it would be better if we could avoid, to make the HTML cleaner. But it should not break anything.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

👍 Looks good

@nicolas-grekas
Copy link
Member Author

Thank you @mpiot.

@nicolas-grekas
Copy link
Member Author

Thank you @Nyholm also

@nicolas-grekas nicolas-grekas merged commit 14e2282 into symfony:3.4 Feb 15, 2018
nicolas-grekas added a commit that referenced this pull request Feb 15, 2018
…le (mpiot, Nyholm)

This PR was merged into the 3.4 branch.

Discussion
----------

[TwigBridge] Apply some changes to support Bootstrap4-stable

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25655 #25635 #24435
| License       | MIT
| Doc PR        | -

Follow up of #25715, see discussion there.

This fixes the following errors:
- Delete form-control-label, don't used in Bootstrap 4
- Replace col-form-legend by col-form-label
- Separate the label and input (before the input was in the label)
- Use form-check-inline to put radio and/or checkboxes inline
- Add support of custom form for radio and checkboxes
- Fix input-group: MoneyType (Issue #25655), PercentType
- Remove form-control duplication (Issue #25635)
- Fix Errors in label (#24435)

Commits
-------

14e2282 Fixed broken tests
cf4e956 [TwigBridge] Apply some changes to support Bootstrap4-stable
@nicolas-grekas nicolas-grekas deleted the mpiot-master branch February 15, 2018 07:59
@Nyholm
Copy link
Member

Nyholm commented Feb 19, 2018

Ping @mpiot, Can you make a PR with the awesome help-block as I mentioned here?
#25715 (comment)

If you do not have time right now that is also very fine. Just tell me.

This was referenced Mar 1, 2018
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.

4 participants