Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[TwigBridge] Apply some changes to support Bootstrap4-stable#26167
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas commentedFeb 13, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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.) |
| {%-setprepend=not (money_patternstarts with'{{') -%} | ||
| {%-setappend=not (money_patternends with'}}') -%} | ||
| {%-ifprependorappend -%} | ||
| <divclass="input-group{{group_class|default('') }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
isgroup_class defined here ?
There was a problem hiding this comment.
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.
| $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"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Here is thecol-form-label added twice. Do we bother?
There was a problem hiding this comment.
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.
9efdb4c to14e2282Compare
Nyholm left a comment
There was a problem hiding this 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 commentedFeb 15, 2018
Thank you@mpiot. |
nicolas-grekas commentedFeb 15, 2018
Thank you@Nyholm also |
…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 testscf4e956 [TwigBridge] Apply some changes to support Bootstrap4-stable
Nyholm commentedFeb 19, 2018
Ping@mpiot, Can you make a PR with the awesome help-block as I mentioned here? If you do not have time right now that is also very fine. Just tell me. |
Uh oh!
There was an error while loading.Please reload this page.
Follow up of#25715, see discussion there.
This fixes the following errors: