Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Bootstrap4 support for Twig form theme#19648
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
HeahDude commentedAug 17, 2016
There were already#16290, but at first sight this PR adds tests and it's a good point :) thanks for working on this! |
hiddewie commentedAug 17, 2016 • 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.
Strange, I did not notice the link of the other PR in the original ticket. Thank you for notifying me. |
hiddewie commentedAug 17, 2016
I do not understand the fail ofhttps://travis-ci.org/symfony/symfony/jobs/152929965 (class missing?). Also, the PR#16290 does not seem up-to-date with the latest classes and HTML structure of BS 4. |
HeahDude commentedAug 17, 2016
the twig bridge now needs the form component to be >= 3.2, so you need to update its |
Simperfit commentedAug 18, 2016
You should consider adding an author tag on the tests class, it's alot of work and we should know who was behind this ;) |
hiddewie commentedSep 18, 2016
What is the review status of this PR? More and more use cases pop up as the Bootstrap 4 alpha becomes more stable and the Bootstrap 3 feature development had been ceased. |
fabpot commentedSep 18, 2016
I would be more than happy to merge this PR before 3.2 end of dev (which happens by the end of the month). |
fabpot commentedSep 18, 2016
At least, this PR needs to be rebased to get rid of the merge commit. |
hiddewie commentedSep 18, 2016
Rebase done. |
fabpot commentedSep 18, 2016
Any Bootstrap 4 users here to help us review this pull request? |
hiddewie commentedSep 22, 2016
Maybe@lyrixx as original creator of the Bootstrap 3 files? |
lyrixx commentedSep 22, 2016
Sorry, I never used bootstrap 4. |
mahono commentedOct 5, 2016
I'll have a look at it this week for a new project and will be able to give feedback by the end of the week. Hope it's not too late. |
eschmar commentedOct 5, 2016
One thing that i've noticed, is that for expanded fields it would be nice to use the Maybe I can have a look on the weekend, but if anyone else can, please go for it! |
hiddewie commentedOct 5, 2016
@eschmar I think the idea of adding a fieldset with a legend to an expanded field is nice. I will look into that. However, the browser support for adding |
mahono commentedOct 7, 2016
I created some forms today using these templates. It looks pretty good to me. Works nice with current Bootstrap 4. I also like the idea of separating templates into an inheritance structure very much. To be honest I'm not enough Bootstrap 4 expert to say that everything works perfect. |
eschmar commentedOct 10, 2016
Thanks@hiddewie for all your efforts! I agree regarding the I just migrated the forms of a smaller project and it works great! The One step even further, would be the possibility to enable the Great work! |
hiddewie commentedOct 10, 2016
@eschmar Could you post your form type with checkboxes? This should be fixed, but I cannot find your problem (any checkboxes I can try render a |
eschmar commentedOct 11, 2016
@hiddewie Excuse me, it was an expanded
|
hiddewie commentedOct 13, 2016
Maybe I am missing something. My entity type is also rendering correctly as Type: HTML: Horizontal layout: |
stof commentedOct 13, 2016
@eschmar make sure that you don't have your own form theme overriding this partially |
| {# Labels #} | ||
| {% block form_label -%} | ||
| {% spaceless %} |
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.
please use whitespace control, not spaceless. Spaceless has a huge performance impact as it processing the template output with a regex and form rendering are using these blocks a lot
| {% block form_row -%} | ||
| {%- if expanded is defined and expanded %} | ||
| {% set element = 'fieldset' %} |
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.
some whitespace control is missing here to ensure we don't have remaining spaces
| {{- form_widget(form) -}} | ||
| {{- form_errors(form) -}} | ||
| </div> | ||
| {##}</{{ element|default('div') }} > |
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.
you have a useless space after the}}
| {%- if expanded is defined and expanded %} | ||
| {% set element = 'fieldset' %} | ||
| {%- endif -%} | ||
| <{{ element|default('div') }} class="form-group row{% if (not compound or force_error|default(false)) and not valid %} has-danger{% endif %}"> |
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.
you have 2 spaces beforeclass while 1 is enough
| {%- endblock checkbox_radio_row %} | ||
| {% block submit_row -%} | ||
| {% spaceless %} |
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.
please don't use spaceless
| {% if required %} | ||
| {% set label_attr = label_attr|merge({class: (label_attr.class|default('') ~ ' required')|trim}) %} | ||
| {% endif %} | ||
| {% if parent_label_class is defined %} |
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.
a bnch of whitespace-control markers are missing here
| <li>{{ error.message }}</li> | ||
| {%- endfor -%} | ||
| </ul> | ||
| {% if form.parent %}</div>{% else %}</div>{% endif %} |
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.
why using a if to display the same in both cases ?
| {% block form_errors -%} | ||
| {% if errors|length > 0 -%} | ||
| {% if form.parent %}<div class="form-control-feedback">{% else %}<div class="alert alert-danger">{% endif %} |
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.
I would use theif only around classes rather than duplicating the whole div
| {% block money_widget -%} | ||
| <div class="input-group"> | ||
| {% set append = money_pattern starts with '{{' %} | ||
| {% if not append %} |
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.
missing whitespace controls here
hiddewie commentedOct 14, 2016
@stof Thank you. Done. Some of the whitespace problems were already in the existing code. They have been fixed anyway. |
eschmar 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.
@hiddewie Sorry for the confusion. There was an issue with how i embedded the form themes. Everything is rendering correctly now!
I tried to find an easy way to mark custom form types to use thefieldset tag, however couldn't find one which didn't involve a lot of changes. An example: I have a customAddressType, where theexpanded option can not be set. But i guess that's a bit out of scope.
This PR is ready for merge, in my opinion.
| {{- form_widget(form) -}} | ||
| {{- form_errors(form) -}} | ||
| </div> | ||
| {##}</{{ element|default('div') }}> |
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.
Is there a reason for this (empty) 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.
The comment was in the previous code (https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_horizontal_layout.html.twig#L34). It was probably put there to avoid whitespace issues (the commit I can find isa35d3d4).
hiddewie commentedOct 17, 2016
@eschmar Thank you. I think your request is a valid feature, but should be put into a new issue because (as you mention) it goes well out of scope of this PR/changeset. |
hiddewie commentedNov 6, 2016
Ping@fabpot. Is there any progress on this feature being in the 3.2 release, or is it being pushed forward to 3.3? |
fabpot commentedNov 6, 2016
I would be for 3.3 now. Apart from feedback from others, anything else to be done before merging? |
hiddewie commentedNov 6, 2016
I've tried to fix all comments up to now, both code style and code content. If there are more new or existing comments I have missed, then I will of course process them. |
alex-bacart commentedDec 22, 2016
@hiddewie margin and padding styles have been changed in Bootstrap v4.0.0-alpha.5 from Error list |
hiddewie commentedDec 23, 2016
@bacart Thanks for noticing me, I've updated the Twig code. |
| {%- if expanded is defined and expanded -%} | ||
| {%- set element = 'fieldset' -%} | ||
| {%- endif -%} | ||
| <{{ element|default('div') }} class="form-group row{% if (not compound or force_error|default(false)) and not valid %} has-danger{% endif %}"> |
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.
You should not add the grid classes (e.g.)row within the form theme. This is mixing concerns of the respective CSS classes. If you would like to make a certainform-group arow, too, you should do this using the frontend compilation (e.g. SASS mixins) and apply proper changes to the addressed form / form group / ...
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.
Seehttp://v4-alpha.getbootstrap.com/components/forms/#using-the-grid. How would you suggest we support horizontal forms in this form theme if not using the defaultform-group &row combination which is required in BS 4?
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.
I'm sorry, I totally missed this being the horizontal one, I was on the path it's the base one.
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.
No problem! Thanks for the review.
alex-bacart commentedJan 11, 2017 • 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.
@hiddewie one more notice: UPD: typo came frombootstrap_3_layout.html.twig. |
hiddewie commentedJan 11, 2017
@bacart This has been fixed. Although in the older code not all blocks were closed with |
hiddewie commentedFeb 21, 2017
I don't get why GitHub closed my PR. It seems it does not recognise my force push of a rebase onto However, the compare is not empty when I do it manually:master...hiddewie:bootstrap4 |
…ereguiluz)This PR was merged into the 3.4 branch.Discussion----------Bootstrap4 support for Twig form theme**This PR is a followup from#19648. That PR was closed because GitHub thought my branch contained no commits after a force push...**| Q | A || --- | --- || Branch? | master || Bug fix? | no || New feature? | yes || BC breaks? | no || Deprecations? | no || Tests pass? | yes || Fixed tickets |#16289 || License | MIT || Doc PR | - |I have made a port of the Twig form theming code for Bootstrap 3 to the α-5 version of Bootstrap 4.- The (inheritance) structure of the form theming files has changed because a number of blocks are the same between BS 3 and 4. They have been migrated to `bootstrap_base_layout.html.twig`. The new tree is as follows:```bootstrap_base_layout.html.twig|-- bootstrap_3_layout.html.twig| `-- bootstrap_3_horizontal_layout.html.twig`-- bootstrap_4_layout.html.twig `-- bootstrap_4_horizontal_layout.html.twig```- Any occurances of `.form-horizontal` have been removed from the BS 4 code.- Checkboxes and radio buttons have been stacked using the `.form-check`, `.form-check-label` and `.form-check-input` classes. There is now no distinction between checkboxes and radio buttons in the markdown.- All layout tests have been added and updated for BS4. The inheritance tree is as follows:```AbstractLayoutTest`-- AbstractBootstrap3LayoutTest |-- AbstractBootstrap3HorizontalLayoutTest `-- AbstractBootstrap4LayoutTest `-- AbstractBootstrap4HorizontalLayoutTest```All tests pass. The classes `FormExtensionBootstrap4LayoutTest` and `FormExtensionBootstrap4HorizontalLayoutTest` have been created similarly to the BS 3 versions.- ~~The label coloring on an validation is not correct. I've made an issue (twbs/bootstrap#20535) of the problem.~~- No [custom form elements](http://v4-alpha.getbootstrap.com/components/forms/#custom-forms) have been used.- A docs PR can be created if this PR is accepted.- The new code might have to be updated if large changes occur in the BS 4 α.Screenshot of BS3 and 4 comparison for the same form:Commits-------f12e588 Removed unneeded wrapping quotes around a Twig key709f134 Removed unneeded wrapping quotes around a Twig key4222d54 Initial commit for Bootstrap 4 form theme, based on Bootstrap 3 form theme
Uh oh!
There was an error while loading.Please reload this page.
I have made a port of the Twig form theming code for Bootstrap 3 to the α-5 version of Bootstrap 4.
bootstrap_base_layout.html.twig.The new tree is as follows:
.form-horizontalhave been removed from the BS 4 code..form-check,.form-check-labeland.form-check-inputclasses. There is now no distinction between checkboxes and radio buttons in the markdown.All tests pass. The classes
FormExtensionBootstrap4LayoutTestandFormExtensionBootstrap4HorizontalLayoutTesthave been created similarly to the BS 3 versions.The label coloring on an validation is not correct. I've made an issue (Form validation labels not coloring twbs/bootstrap#20535) of the problem.Screenshot of BS3 and 4 comparison for the same form: