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#21751
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
stof commentedFeb 24, 2017
Looks like you removed any commits once again... |
stof commentedFeb 24, 2017
btw, you could push to the same branch name again and reopen the existing PR instead of creating a new one |
hiddewie commentedFeb 24, 2017
I still have no idea why GitHub did not show my initial patch and did not allow me to reopen the previous PR. |
fabpot commentedMar 5, 2017
Anyone willing to review this one? @symfony/deciders |
fabwu commentedAug 10, 2017
@hiddewie Is it possible to usecustom checkboxes with this PR? |
hiddewie commentedAug 10, 2017
At the moment no, this should be a choice for the end user designing their web application using Bootstrap. However, I would be happy to create a follow-up PR if this has been accepted containing one extra template using the Bootstrap 4 form template, but with custom Bootstrap checkboxes/radio buttons. |
fabwu commentedAug 10, 2017
@hiddewie Ok thanks for your quick reply. Hope this gets merged soon! |
hiddewie commentedAug 10, 2017
I hope so too. The PR gets rebased every month or so, but originally it was submitted for 3.2. |
javiereguiluz commentedAug 11, 2017
Bootstrap 4 has just published its first beta version:https://github.com/twbs/bootstrap/releases/tag/v4.0.0-beta Could we please review if they made any change that affects to this pull request? Thanks! |
hiddewie commentedAug 14, 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.
Several small things have been updated for the beta version of Bootstrap 4:
I do not understand the single failing test in Travis CI, as the class seems to be defined correctly. |
robfrawley commentedAug 15, 2017
@hiddewie That is the |
hiddewie commentedAug 15, 2017
@robfrawley The I don't see how upping the dependency would solve this problem, as it needsmy updated PR code, both for the Twig Bridge and the Form component. How to solve this? |
robfrawley commentedAug 15, 2017
The dependencies do matter; I'm not 100% sure how this is handled internally but see#23451 (comment) where I had to deal with a similar issue, as well. |
hiddewie commentedAug 15, 2017
Ah, so I guess that the test with the missing abstract class will have a green light after the PR is merged, because the downloaded dependency of |
xabbuh commentedAug 22, 2017
@hiddewie You need to rebase your PR against the |
hiddewie commentedAug 23, 2017
@xabbuh Done. The single failing test with |
xabbuh commentedAug 24, 2017
You need to update the version constraint for "require-dev": {"symfony/form":"~3.4|~4.0"},"conflict": {"symfony/form":"<3.4"} |
hiddewie commentedAug 24, 2017
@xabbuh Yes, thank you. Now the |
MrMitch 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.
The style for file inputs seems to be missing.
| {%- set attr = attr|merge({class: (attr.class|default('') ~ ' form-control')|trim}) -%} | ||
| {% endif %} | ||
| {{- parent() -}} | ||
| {%- endblock form_widget_simple %} |
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.
For file inputs, boostrap recommendsswitching the class from.form-control to.form-control-file and this seems to be missing from this theme.
You can include this in by appending '-file' to the default 'form-control' class if the widget type is set to 'file' and slightly changing the type detection condition :
{%blockform_widget_simple -%} {%iftypeis notdefinedortype!='hidden' %} {%-setattr=attr|merge({class: (attr.class|default('') ~' form-control' ~ (type|default('') =='file' ?'-file' :''))|trim}) -%} {%endif %} {{-parent() -}}{%-endblockform_widget_simple %}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.
Thank you for the comment.
I've split that widget into a BS 3 and BS 4 variant, and used your suggestion. The layout test for the file input has also been updated.
…themeFixed Bootstrap 4 error outputUpdated form errors to look correctlyCranked Twig Bridge Composer version to ~3.2Added @ author PHPdoc tag to BS 4 test classesFixed small items, and added fieldset/legend support- Fixed form class for File type- Added fieldset element for expanded form types- Added legend element as label for expanded form types- Fixed horizontal form classes for labelsAdded test for legend form labelFixed form label coloring on validation errorsFixed concrete Bootstrap layout test classes to use new codeFixed tests for form-control-label class on form labelsFixed a typo in using old Bootstrap 4 concrete test codeProcessed proposed changes from stofProcessed proposed changes in bootstrap base layout from stofUpdated margin-top for error list in the alpha-5 version of BS 4Fixed {% endblock ... %} and fixed BS error class in testFixed TwigRenderer => FormRenderer code changeMinor fixed for radio/checkboxes and fixed validation feedbackAdded ~3.4 require of symfony/form (and <3.4 conflict) to Twig Bridgecomposer.jsonUpdated Boostrap 4 file input to have class .form-control-file
javiereguiluz 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 you did a great work here. Thank you!
Considering thatBootstrap 4 is in beta since August 2017, this is now safe to be merged.
Nyholm commentedSep 26, 2017
I've just had a quick look at the screenshots. I have a few things regarding accessibility and errors. But I guess that is no blocker for getting this merged. |
silverbackdan commentedSep 26, 2017
Just as a note on the custom options - I have an implementation when BS4 was in alpha6 which also required some form type extensions to give the options as to whether a user wants custom inputs or not. Extensions are here:https://github.com/silverbackis/SilverbackPublishing/tree/master/src/SyliusUiBootstrapBundle/Form/Extension This was my layout: I think I left bits out and this PR is far superior as a template, but I thought I may comment this to show how the custom options may be implemented in a follow-up PR after this is merged. @hiddewie were you thinking to implement the custom option is a way where it could be configured with options? |
fabpot commentedOct 1, 2017
Thank you@hiddewie. |
…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
hiddewie commentedOct 9, 2017
Thank you@fabpot! |
This PR is a followup from#19648. That PR was closed because GitHub thought my branch contained no commits after a force push...
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: