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

Form theme: support Bootstrap 4 custom switches #33954

Merged
merged 1 commit into from Nov 3, 2019

Conversation

romaricdrigon
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? no
New feature? yes
Deprecations? no
Tickets /
License MIT
Doc PR symfony/symfony-docs#12464

Hello,

At the moment, Symfony form theme supports custom checkboxes through an extra class in label_attr.
Bootstrap4 introduced also custom switches, which has exactly the same HTML markup, but use a different class. This PR slightly modify bootstrap_4_layout to handle it.

image

Some reasons why I think supporting those have its place in Symfony:

  • those are getting common in UI right now, it is a common use case
  • it is complementary to normal checkboxes, and works the same way: required attribute, validation error, and so on are supported immediately
  • implementing it yourself in your form theme is actually tricky, because of the way checkbox are handled (ie., form_label called inside form_widget with a { widget: parent() }). You have to overwrite the whole fragment, otherwise you get an infinite recursion.

Finally, some screenshots and code examples.

Custom checkbox (as at the moment):
image

    ->add('test', CheckboxType::class, [
        'label_attr' => [
            'class' => 'checkbox-custom',
        ],
    ])

Custom switch (proposed):
image

    ->add('test', CheckboxType::class, [
        'label_attr' => [
            'class' => 'switch-custom',
        ],
    ])

@javiereguiluz
Copy link
Member

I love this ... but I have two questions:

  • The "discoverability" is not great because you are changing the form field look and feel by setting a CSS class in the label 🤔 . Ideally we'd have a ToggleType or SwitchType form field.
  • This makes the form themes "different". Until now, the only difference between themes was their design ... but this adds a new feature that it's exclusive to Bootstrap theme.

@romaricdrigon
Copy link
Contributor Author

I'm seeing it as a checkbox "skin". It still behaves exactly as a checkbox, both functionality wise (for the user) and in PHP code (for the developer). So imo adding a SwitchType would be cumbersome, for instance it would likely require developers to modify their Form extensions and so on. I think it should stay only a presentation concern.

I think the feature is not quite exclusive to Bootstrap, other framework could read that attribute and implement switches too (for instance, Foundation took a similar approach). Indeed, right Bootstrap has a better integration such as form help (which is displayed as a normal div for Foundation), invalid form fields, etc., but we don't break any other form themes or prevent them to use it too.

Copy link
Member

@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

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

@romaricdrigon your comment made me change my mind. Although we're on the edge here, it's true that this is just "a skin", even if the perceived behavior changes. Thanks!

@@ -10,6 +10,8 @@ CHANGELOG
* the `LintCommand` lints all the templates stored in all configured Twig paths if none argument is provided
* deprecated accepting STDIN implicitly when using the `lint:twig` command, use `lint:twig -` (append a dash) instead to make it explicit.
* added `--show-deprecations` option to the `lint:twig` command
* added support for Bootstrap4 switches, use `switch-custom` as `label_attr` in a `CheckboxType`

Copy link
Member

Choose a reason for hiding this comment

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

the additional blank line should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, that's fixed

@fabpot
Copy link
Member

fabpot commented Nov 3, 2019

Thank you @romaricdrigon.

fabpot added a commit that referenced this pull request Nov 3, 2019
…icdrigon)

This PR was merged into the 4.4 branch.

Discussion
----------

Form theme: support Bootstrap 4 custom switches

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | /
| License       | MIT
| Doc PR        | symfony/symfony-docs#12464

Hello,

At the moment, Symfony form theme supports [custom checkboxes](https://getbootstrap.com/docs/4.3/components/forms/#checkboxes) through an extra class in `label_attr`.
Bootstrap4 introduced also [custom switches](https://getbootstrap.com/docs/4.3/components/forms/#switches), which has exactly the same HTML markup, but use a different class. This PR slightly modify `bootstrap_4_layout` to handle it.

![image](https://user-images.githubusercontent.com/919405/66651725-0eaa3100-ec34-11e9-8b68-94324730ac80.png)

Some reasons why I think supporting those have its place in Symfony:
 - those are getting common in UI right now, it is a common use case
 - it is complementary to normal checkboxes, and works the same way: required attribute, validation error, and so on are supported immediately
 - implementing it yourself in your form theme is actually tricky, because of the way checkbox are handled (ie., `form_label` called inside `form_widget` with a `{ widget: parent() }`). You have to overwrite the whole fragment, otherwise you get an infinite recursion.

Finally, some screenshots and code examples.

Custom checkbox (as at the moment):
![image](https://user-images.githubusercontent.com/919405/66652982-41a1f400-ec37-11e9-813f-4b39087e89e7.png)
```php
    ->add('test', CheckboxType::class, [
        'label_attr' => [
            'class' => 'checkbox-custom',
        ],
    ])
```
Custom switch (proposed):
![image](https://user-images.githubusercontent.com/919405/66652902-1919fa00-ec37-11e9-98f3-9340b01b2335.png)
```php
    ->add('test', CheckboxType::class, [
        'label_attr' => [
            'class' => 'switch-custom',
        ],
    ])
```

Commits
-------

99f59e2 Supporting Bootstrap 4 custom switches
@fabpot fabpot merged commit 99f59e2 into symfony:4.4 Nov 3, 2019
@romaricdrigon romaricdrigon deleted the feat-switch branch November 4, 2019 18:38
yceruto added a commit that referenced this pull request Nov 6, 2019
This PR was merged into the 4.4 branch.

Discussion
----------

[TwigBridge] Fix switch-custom changelog entry

| Q             | A
| ------------- | ---
| Branch?       | 4.4 <!-- see below -->
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Relates to #33954
| License       | MIT
| Doc PR        | N/A

As it doesn't really mention `switch-custom` is a css class.

Commits
-------

b03b7f4 [TwigBridge] Fix switch-custom changelog entry
This was referenced Nov 12, 2019
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
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.

None yet

6 participants