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

Bootstrap4 support for Twig form theme #21751

Merged
merged 3 commits into from Oct 1, 2017

Conversation

Projects
None yet
@hiddewie
Contributor

hiddewie commented Feb 24, 2017

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 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:

1

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Feb 24, 2017

Member

Looks like you removed any commits once again...

Member

stof commented Feb 24, 2017

Looks like you removed any commits once again...

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Feb 24, 2017

Member

btw, you could push to the same branch name again and reopen the existing PR instead of creating a new one

Member

stof commented Feb 24, 2017

btw, you could push to the same branch name again and reopen the existing PR instead of creating a new one

@hiddewie hiddewie reopened this Feb 24, 2017

@hiddewie

This comment has been minimized.

Show comment
Hide comment
@hiddewie

hiddewie Feb 24, 2017

Contributor

I still have no idea why GitHub did not show my initial patch and did not allow me to reopen the previous PR.
In the previous PR I am using the same branch, for which the GitHub diff shows (and showed) the single commit which was my PR patch.
I will keep this one open, as the previous PR still shows 0 commits and no diff.

Contributor

hiddewie commented Feb 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.
In the previous PR I am using the same branch, for which the GitHub diff shows (and showed) the single commit which was my PR patch.
I will keep this one open, as the previous PR still shows 0 commits and no diff.

@nicolas-grekas nicolas-grekas added this to the 3.x milestone Feb 25, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Mar 5, 2017

Member

Anyone willing to review this one? @symfony/deciders

Member

fabpot commented Mar 5, 2017

Anyone willing to review this one? @symfony/deciders

@javiereguiluz javiereguiluz self-requested a review Mar 6, 2017

@fabwu

This comment has been minimized.

Show comment
Hide comment
@fabwu

fabwu Aug 10, 2017

@hiddewie Is it possible to use custom checkboxes with this PR?

fabwu commented Aug 10, 2017

@hiddewie Is it possible to use custom checkboxes with this PR?

@hiddewie

This comment has been minimized.

Show comment
Hide comment
@hiddewie

hiddewie Aug 10, 2017

Contributor

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.

Contributor

hiddewie commented Aug 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

This comment has been minimized.

Show comment
Hide comment
@fabwu

fabwu Aug 10, 2017

@hiddewie Ok thanks for your quick reply. Hope this gets merged soon!

fabwu commented Aug 10, 2017

@hiddewie Ok thanks for your quick reply. Hope this gets merged soon!

@hiddewie

This comment has been minimized.

Show comment
Hide comment
@hiddewie

hiddewie Aug 10, 2017

Contributor

I hope so too. The PR gets rebased every month or so, but originally it was submitted for 3.2.

Contributor

hiddewie commented Aug 10, 2017

I hope so too. The PR gets rebased every month or so, but originally it was submitted for 3.2.

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Aug 11, 2017

Member

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!

Member

javiereguiluz commented Aug 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!

@Guite Guite referenced this pull request Aug 11, 2017

Open

Upgrade to Bootstrap 4.0 #3530

0 of 1 task complete
@hiddewie

This comment has been minimized.

Show comment
Hide comment
@hiddewie

hiddewie Aug 14, 2017

Contributor

Several small things have been updated for the beta version of Bootstrap 4:

  • The div layout had to be updated slightly for radio buttons and checkboxes
  • The form validation has been updated with the new classes. See twbs/bootstrap#23426.

I do not understand the single failing test in Travis CI, as the class seems to be defined correctly.

Contributor

hiddewie commented Aug 14, 2017

Several small things have been updated for the beta version of Bootstrap 4:

  • The div layout had to be updated slightly for radio buttons and checkboxes
  • The form validation has been updated with the new classes. See twbs/bootstrap#23426.

I do not understand the single failing test in Travis CI, as the class seems to be defined correctly.

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Aug 15, 2017

Contributor

I do not understand the single failing test in Travis CI, as the class seems to be defined correctly.

@hiddewie That is the deps=low test; it is possible an older version of the twig-bridge is being pulled in. Possibly, you need to up the required dependency?

Contributor

robfrawley commented Aug 15, 2017

I do not understand the single failing test in Travis CI, as the class seems to be defined correctly.

@hiddewie That is the deps=low test; it is possible an older version of the twig-bridge is being pulled in. Possibly, you need to up the required dependency?

@hiddewie

This comment has been minimized.

Show comment
Hide comment
@hiddewie

hiddewie Aug 15, 2017

Contributor

@robfrawley The deps=low build config confuses me. In this PR, I added content in the Twig Bridge and in the Form component (the base/abstract classes for the Bootstrap tests). However, it seems to download a stable version from Composer of the Form component, which of course does not contain my test classes, and causes the error for the Twig Bridge tests.

I don't see how upping the dependency would solve this problem, as it needs my updated PR code, both for the Twig Bridge and the Form component. How to solve this?

Contributor

hiddewie commented Aug 15, 2017

@robfrawley The deps=low build config confuses me. In this PR, I added content in the Twig Bridge and in the Form component (the base/abstract classes for the Bootstrap tests). However, it seems to download a stable version from Composer of the Form component, which of course does not contain my test classes, and causes the error for the Twig Bridge tests.

I don't see how upping the dependency would solve this problem, as it needs my updated PR code, both for the Twig Bridge and the Form component. How to solve this?

@robfrawley

This comment has been minimized.

Show comment
Hide comment
@robfrawley

robfrawley Aug 15, 2017

Contributor

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.

Contributor

robfrawley commented Aug 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

This comment has been minimized.

Show comment
Hide comment
@hiddewie

hiddewie Aug 15, 2017

Contributor

It'll be green when the PR will be merged.

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 symfony/form is version 3.4.x-dev, the same version for which this PR is proposed.

Contributor

hiddewie commented Aug 15, 2017

It'll be green when the PR will be merged.

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 symfony/form is version 3.4.x-dev, the same version for which this PR is proposed.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Aug 22, 2017

Member

@hiddewie You need to rebase your PR against the 3.4 branch and then change the target path of the PR on GitHub too.

Member

xabbuh commented Aug 22, 2017

@hiddewie You need to rebase your PR against the 3.4 branch and then change the target path of the PR on GitHub too.

@hiddewie hiddewie changed the base branch from master to 3.4 Aug 23, 2017

@hiddewie

This comment has been minimized.

Show comment
Hide comment
@hiddewie

hiddewie Aug 23, 2017

Contributor

@xabbuh Done. The single failing test with deps=low is again the one which requires my new abstract class in the Forms bundle.

Contributor

hiddewie commented Aug 23, 2017

@xabbuh Done. The single failing test with deps=low is again the one which requires my new abstract class in the Forms bundle.

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Aug 24, 2017

Member

You need to update the version constraint for symfony/form in the require-dev section of the TwigBridge as well as the conflict rule like this:

"require-dev": {
    "symfony/form": "~3.4|~4.0"
},
"conflict": {
    "symfony/form": "<3.4"
}
Member

xabbuh commented Aug 24, 2017

You need to update the version constraint for symfony/form in the require-dev section of the TwigBridge as well as the conflict rule like this:

"require-dev": {
    "symfony/form": "~3.4|~4.0"
},
"conflict": {
    "symfony/form": "<3.4"
}
@hiddewie

This comment has been minimized.

Show comment
Hide comment
@hiddewie

hiddewie Aug 24, 2017

Contributor

@xabbuh Yes, thank you. Now the deps=high is failing, again because the abstract class is missing. Probably because the branch targets 3.4 and the 4.0 version of symfony/form is being installed.

Contributor

hiddewie commented Aug 24, 2017

@xabbuh Yes, thank you. Now the deps=high is failing, again because the abstract class is missing. Probably because the branch targets 3.4 and the 4.0 version of symfony/form is being installed.

@MrMitch

The style for file inputs seems to be missing.

hiddewie and others added some commits Feb 24, 2017

Initial commit for Bootstrap 4 form theme, based on Bootstrap 3 form …
…theme

Fixed Bootstrap 4 error output

Updated form errors to look correctly

Cranked Twig Bridge Composer version to ~3.2

Added @ author PHPdoc tag to BS 4 test classes

Fixed 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 labels

Added test for legend form label

Fixed form label coloring on validation errors

Fixed concrete Bootstrap layout test classes to use new code

Fixed tests for form-control-label class on form labels

Fixed a typo in using old Bootstrap 4 concrete test code

Processed proposed changes from stof

Processed proposed changes in bootstrap base layout from stof

Updated margin-top for error list in the alpha-5 version of BS 4

Fixed {% endblock ... %} and fixed BS error class in test

Fixed TwigRenderer => FormRenderer code change

Minor fixed for radio/checkboxes and fixed validation feedback

Added ~3.4 require of symfony/form (and <3.4 conflict) to Twig Bridge
composer.json

Updated Boostrap 4 file input to have class .form-control-file
@javiereguiluz

@hiddewie you did a great work here. Thank you!

Considering that Bootstrap 4 is in beta since August 2017, this is now safe to be merged.

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Sep 26, 2017

Member

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.

Member

Nyholm commented Sep 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

This comment has been minimized.

Show comment
Hide comment
@silverbackdan

silverbackdan Sep 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:
https://github.com/silverbackis/SilverbackPublishing/blob/master/src/SyliusUiBootstrapBundle/Resources/views/Form/bootstrap_4_layout.html.twig

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?

silverbackdan commented Sep 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:
https://github.com/silverbackis/SilverbackPublishing/blob/master/src/SyliusUiBootstrapBundle/Resources/views/Form/bootstrap_4_layout.html.twig

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

fabpot approved these changes Oct 1, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 1, 2017

Member

Thank you @hiddewie.

Member

fabpot commented Oct 1, 2017

Thank you @hiddewie.

@fabpot fabpot merged commit f12e588 into symfony:3.4 Oct 1, 2017

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

fabpot added a commit that referenced this pull request Oct 1, 2017

feature #21751 Bootstrap4 support for Twig form theme (hiddewie, javi…
…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:

![1](https://cloud.githubusercontent.com/assets/1073881/17732594/dfcb50d6-6472-11e6-8e96-c46987809322.PNG)

Commits
-------

f12e588 Removed unneeded wrapping quotes around a Twig key
709f134 Removed unneeded wrapping quotes around a Twig key
4222d54 Initial commit for Bootstrap 4 form theme, based on Bootstrap 3 form theme
@hiddewie

This comment has been minimized.

Show comment
Hide comment
@hiddewie

hiddewie Oct 9, 2017

Contributor

Thank you @fabpot!

Contributor

hiddewie commented Oct 9, 2017

Thank you @fabpot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment