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

[TwigBridge] Added form_twitter_bootstrap_x.y.z_layout.html.twig #10272

Merged
merged 1 commit into from Oct 5, 2014

Conversation

Projects
None yet
@lyrixx
Copy link
Member

commented Feb 15, 2014

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR not yet

Doc:

# config.yml
twig:
    form:
        resources:
            - form_twitter_bootstrap_2.3.x_layout.html.twig

or

{% form_theme form 'form_twitter_bootstrap_2.3.x_layout.html.twig' %}

{{ form(form) }}
@aitboudad

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2014

I don't think it be a good idea,
there is many good bundles like https://github.com/phiamo/MopaBootstrapBundle for TB3.

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2014

@aitboudad I'm working on a simple integration of TB3. Therefore, as TW is almost a standard, IMHO we can add some optional layout into the twig bridge.

@aitboudad

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2014

@lyrixx well if we will add TB3, I propose also to add:

But the problem is that there are many versions, for example TB2.

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Feb 15, 2014

@aitboudad Feel free to open a PR ;)

@tadcka

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2014

-1

@henrikbjorn

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2014

+1

1 similar comment
@mykiwi

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2014

👍

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2014

@aitboudad BTW, this code should not belong to a bundle, because it is not re-usable (in a silex application for instance).

@Renrhaf

This comment has been minimized.

Copy link

commented Feb 16, 2014

+1

1 similar comment
@ahilles107

This comment has been minimized.

Copy link

commented Feb 16, 2014

👍

@wouterj

This comment has been minimized.

Copy link
Member

commented Feb 16, 2014

-1, this is something the community should be doing in contributed bundles, it does not belong into core imo

@norzechowicz

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2014

Like @wouterj said, I think this such implementation should be done by community in separated bundle (or something like that). It's not a twig bridge task to integrate Twitter Bootstrap with symfony form.

@henrikbjorn

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2014

I dont get the negativity for adding this, how many are going to install a Bundle to get a template for the form component?

Currently there is two layouts, table and div adding theese are good usecases on how to customize the form layouts. Which IMO is a part of forms that are not documented that good.

@wouterj

This comment has been minimized.

Copy link
Member

commented Feb 16, 2014

Adding an opinionated thing to the core just because the docs are not good is a no-go imo. Then the docs should be improved instead.

And indeed, a package would be a better fit instead of a bundle. The core just needs to be as less opinionated as possible, which did go pretty well until this PR :)

@henrikbjorn

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2014

Well i would not see a Bridge as a core thing. It is an extension for other libraries, none said it had to be php libraries, that is the same as saying we cant have a Doctrine bridge because we are opiniated about using Doctrine over Propel.

@wouterj

This comment has been minimized.

Copy link
Member

commented Feb 16, 2014

@henrikbjorn if we created a BootstrapBridge, you would not see me commenting with -1. This bridge is about implementing twig into Symfony, not twitter bootstrap.

@garak

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2014

+1

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Feb 16, 2014

👍

Deciding if something like this belongs to the core is always tricky. But please, consider the following two pro-core arguments:

  • There are 44 public Symfony2 bundles related to Bootstrap. If something is that popular and that needed in real-world applications, I guess it could go in the core (in my opinion, if there are more than 30 bundles to do something, then Symfony is lacking that feature).
  • If Bootstrap theme is included in the core, there is no obligation to include themes for other CSS frameworks (as suggested by @aitboudad). Bootstrap is, by a huge distance, the most used and most popular framework (just look at the GitHub stars, watchs and forks). In practice, Bootstrap has become as standard as designing with <div> and <table>, the other two existing form themes.
@saro0h

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2014

👍

Can't agree more with Javier!

@aitboudad

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2014

@lyrixx you should add "twbs/bootstrap" in composer suggested packages.

@henrikbjorn

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2014

@aitboudad ohh please no. There are already waaayyy too many suggested packages and it have no value (only the very first time a package is installed).

@lmcd

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2014

👍

@L0rD59

This comment has been minimized.

Copy link

commented Feb 16, 2014

-1

@wouterj

This comment has been minimized.

Copy link
Member

commented Feb 16, 2014

@javiereguiluz as a matter of facts, there are 33 bundles related to jQuery, 32 about payment, 67 about admin generators/interfaces, 38 about caching, 39 are shop/cart related, etc. Symfony core will get pretty big this way 😉

@hacfi

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2014

-1

For the same reasons @wouterj mentioned (and more useful/general things have been rejected to keep the core clean).

@docteurklein

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2014

@lyrixx well, bundle templates/resources/code are reusable into silex. They just aren't auto discovered and you can't register its bundle.

@henrikbjorn there is aslo a propel1 bridge because it has been decided to officially support it.

So the question is to know if symfony officially supports bootstrap, in which case having a form theme as a bridge makes sense.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Feb 17, 2014

@wouterj yes, there are a lot of bundles for a lot of things: if jQuery bundles are just for including it, they are unnecessary (thanks to grunt, bower, requireJS); payments/cart/shop bundles are too specific (useful only for e-ecommerce sites); caching bundles are non-essential because Symfony resolves this problem well; 67 admin generator bundles is a clear proof that Symfony lacks a real generator, which is needed by everyone.

@hacfi the core isn't clean and it provides a lot of things that nobody needs, such as the percent and birthday fields and the IP and ISSN validators.

This discussion about Bootstrap reminds me the Apache discussion. A few years ago, Symfony decided to include a .htaccess file. It wasn't strictly necessary, but it was a nice addition for real developers solving real-world problems. And Symfony only included an .htaccess because Apache was at the moment the indisputable leader. A few years later Symfony is now evaluating if it's necessary to keep that file in the coming versions.

It would be nice to support Bootstrap-theme forms out-of-the-box without the need to install or configure anything.

@MaksBaum

This comment has been minimized.

Copy link

commented Feb 17, 2014

-1
Adding support for only one css framework isn't good idea.
It's not a problem to add one file by developer to use bootstrap.

@javiereguiluz

This comment has been minimized.

Copy link
Member

commented Feb 17, 2014

@MaksBaum Bootstrap is not "one CSS framework" but the most popular framework by a huge distance, the most popular project in GitHub history, and one of the projects that have revolutionized the frontend.

And don't forget that Symfony already has a lot of features aimed for only one product, service, or vendor:

  • Symfony clearly prioritizes Doctrine over Propel and completely ignores any other ORM or ActiveRecord library.
  • Symfony provides a .htaccess for Apache and provides nothing for Nginx and other web servers.
  • Symfony only cares about PHP and Twig template engines and forgets about Smarty, Blade, etc.
  • The credit card validator only validates a few selected card issuers.
  • SwiftMailer provides a nice shortcut to send emails easily with Gmail, while ignoring any other web email provider.
@webmozart

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2014

I'm slightly in favor of adding this, for the reasons outlined by @javiereguiluz. However, why are you building a completely new Bootstrap theme here instead of building upon a tried and proven one from one of the various Bootstrap bundles out there?

@liverbool

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2014

-1

@guilhermeblanco

This comment has been minimized.

Copy link
Contributor

commented Sep 24, 2014

@fabpot @javiereguiluz @webmozart @henrikbjorn Create a Symfony Bridge for Bootstrap (not inside Twig) and I'm sure half of the complains are gonna disappear.

@dirkluijk

This comment has been minimized.

Copy link

commented Sep 24, 2014

1 similar comment
@ioleo

This comment has been minimized.

Copy link

commented Sep 24, 2014

@webmozart

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2014

The collection type seems to be missing from your examples? What about rendering a collection with rows using "inline" layouts?

@lyrixx Any feedback to that?

@avevlad

This comment has been minimized.

Copy link

commented Sep 25, 2014

👍

1 similar comment
@messfromspace

This comment has been minimized.

Copy link

commented Sep 25, 2014

👍

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Sep 25, 2014

The collection type seems to be missing from your examples? What about rendering a collection with rows using "inline" layouts?

@webmozart by inline, you mean horizontal ?

Anyway, indeed I forgot the collection type, But I added a sub-form. This is quit similar ;)

I will add more sample.

@webmozart

This comment has been minimized.

Copy link
Contributor

commented Sep 25, 2014

@lyrixx Cool, thanks! :)

{{- parent() -}}
{%- endblock textarea_widget %}

{% block submit_widget -%}

This comment has been minimized.

Copy link
@manuelj555

manuelj555 Oct 1, 2014

Please add a block for the button_widget.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 1, 2014

Author Member

Why is it needed ? You can still override it.

This comment has been minimized.

Copy link
@manuelj555

manuelj555 Oct 1, 2014

To maintain a uniform design to use as both submit and button

This comment has been minimized.

Copy link
@manuelj555

manuelj555 Oct 1, 2014

I am using this theme, and i created a form with one submit and one button types, in the view the submit is pretty printed, but the button does not.

Should not paint boths?

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 1, 2014

Author Member

I'm not sure to understand what you want. Can you paste here the wanted block? Thanks ;)

This comment has been minimized.

Copy link
@manuelj555

manuelj555 Oct 1, 2014

Sorry for my bad English :/

{% block button_widget -%}
    {% set attr = attr|merge({class: (attr.class|default('') ~ ' btn')|trim}) %}
    {{- parent() -}}
{%- endblock %}

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 1, 2014

Author Member

ok, Got it. Thanks.

This comment has been minimized.

Copy link
@lyrixx

lyrixx Oct 1, 2014

Author Member

done.

This comment has been minimized.

Copy link
@manuelj555

@lyrixx lyrixx force-pushed the lyrixx:form-tw branch from 2a30055 to cfc04a6 Oct 1, 2014

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2014

@webmozart

vertical:
vertical
horizontal
horizontal
main vertical collection horizontal
main vertical collection horizontal
main vertical collection inline
main vertical collection inline
main horizontal collection vertical
main horizontal collection vertical
main horizontal collection inline
main horizontal collection inline

@fabpot fabpot referenced this pull request Oct 5, 2014

Closed

Bootstrap theme for forms #4296

@fabpot

This comment has been minimized.

Copy link
Member

commented Oct 5, 2014

Thank you @lyrixx.

@fabpot fabpot merged commit cfc04a6 into symfony:master Oct 5, 2014

0 of 2 checks passed

continuous-integration/travis-ci The Travis CI build failed
Details
default Success: fabbot — Failure: Travis
Details

fabpot added a commit that referenced this pull request Oct 5, 2014

feature #10272 [TwigBridge] Added form_twitter_bootstrap_x.y.z_layout…
….html.twig (lyrixx)

This PR was merged into the 2.6-dev branch.

Discussion
----------

[TwigBridge] Added form_twitter_bootstrap_x.y.z_layout.html.twig

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | not yet

Doc:

``` yaml
# config.yml
twig:
    form:
        resources:
            - form_twitter_bootstrap_2.3.x_layout.html.twig
```

or

```jinja

{% form_theme form 'form_twitter_bootstrap_2.3.x_layout.html.twig' %}

{{ form(form) }}
```

Commits
-------

cfc04a6 [TwigBridge] Added form_twitter_bootstrap_layout.html.twig

@lyrixx lyrixx deleted the lyrixx:form-tw branch Oct 5, 2014

@Macsch15

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2014

Awesome. If somebody uses Glyphicons or FontAwesome below example which replaces default list style to icon.

Replace:

{% block form_errors -%}
    {% if errors|length > 0 %}
    {% if form.parent %}<span class="help-block">{% else %}<div class="alert alert-danger">{% endif %}
        {{- parent() -}}
    {% if form.parent %}</span>{% else %}</div>{% endif %}
    {% endif %}
{%- endblock form_errors %}

With:

{% block form_errors -%}
    {% if errors|length > 0 -%}
    {% if form.parent %}<span class="help-block">{% else %}<div class="alert alert-danger">{% endif %}
    <ul class="list-unstyled">
        {%- for error in errors -%}
            <li><span class="glyphicon glyphicon-exclamation-sign"></span> {{ error.message|trans({}, translation_domain) }}</li>
        {%- endfor -%}
    </ul>
    {% if form.parent %}</span>{% else %}</div>{% endif %}
    {%- endif %}
{%- endblock form_errors %}

Before:
2014-10-07 11-55-50

After:
2014-10-07 11-55-07

@mykiwi

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2014

@Macsch15 you should create a PR for this.

@Macsch15

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2014

PR: #12164

@simplyniceweb

This comment has been minimized.

Copy link

commented Oct 7, 2014

I'm wondering if there's any tutorial on how can i add this using Silex? Or any tutorials that implement something similar to this project into Silex?

@lyrixx

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2014

@simplynice25 The documentation is coming ;)

@simplyniceweb

This comment has been minimized.

Copy link

commented Oct 7, 2014

@lyrixx thanks! Will wait! :D

@hellomedia

This comment has been minimized.

Copy link

commented Oct 17, 2014

Thanks for this PR ! yeah for pragmatic.

@joelpittet

This comment has been minimized.

Copy link

commented Oct 18, 2014

-1,
Echoing "this is something the community should be doing in contributed bundles, it does not belong into core imo"

We had a very similar and long discussion in Drupal 8 core issue queue:
https://www.drupal.org/node/1801582

@wouterj

This comment has been minimized.

Copy link
Member

commented Oct 18, 2014

@joelpittet it does not make sense to start the discussion again. The code is merged and the decision is made.

Maintainers, this may be a good issue to lock?

@symfony symfony locked and limited conversation to collaborators Oct 18, 2014

@lyrixx

This comment has been minimized.

can you open a PR or an issue? Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.