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] Fix the money form type render with Bootstrap3 #19426

Merged
merged 1 commit into from Jul 25, 2016

Conversation

Projects
None yet
5 participants
@Th3Mouk
Contributor

Th3Mouk commented Jul 25, 2016

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets Part fixing #19424
License MIT
Doc PR none

There is a confusion between the variable naming, and the result expected.

When prepend variable is false, the currency symbol must be add after the widget.
When the money_patternstarts with {{, prepend variable must be false.

@@ -21,8 +21,8 @@
{% block money_widget -%}
<div class="input-group">
{% set prepend = '{{' == money_pattern[0:2] %}
{% if not prepend %}
{% set prepend = '{{' != money_pattern[0:2] %}

This comment has been minimized.

@javiereguiluz

javiereguiluz Jul 25, 2016

Member

I love the readability of the starts with Twig operator. If you agree, you could change this code as follows:

{% block money_widget -%}
    <div class="input-group">
        {% set append = money_pattern starts with '{{' %}
        {% if not append %}
            <span class="input-group-addon">{{ money_pattern|replace({ '{{ widget }}':''}) }}</span>
        {% endif %}
        {{- block('form_widget_simple') -}}
        {% if append %}
            <span class="input-group-addon">{{ money_pattern|replace({ '{{ widget }}':''}) }}</span>
        {% endif %}
    </div>
{%- endblock money_widget %}

This comment has been minimized.

@Th3Mouk

Th3Mouk Jul 25, 2016

Contributor

I love too, i didn't think about it.

@javiereguiluz: I've searched long time ago the not starts with feature but seems impossible right ?

This comment has been minimized.

@stof

stof Jul 25, 2016

Member

no it is not. It needs to be not money_pattern starts with '...'

Fix the money form type render with Bootstrap3
There is a confusion between the variable naming, and the result expected.

When prepend variable is false, the currency symbol must be add after the widget.
When the money_patternstarts with {{, prepend variable must be false.
@javiereguiluz

This comment has been minimized.

Member

javiereguiluz commented Jul 25, 2016

👍

Status: reviewed

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jul 25, 2016

Thank you @Th3Mouk.

@nicolas-grekas nicolas-grekas merged commit 637a441 into symfony:2.7 Jul 25, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jul 25, 2016

bug #19426 [Form] Fix the money form type render with Bootstrap3 (Th3…
…Mouk)

This PR was merged into the 2.7 branch.

Discussion
----------

[Form] Fix the money form type render with Bootstrap3

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | Part fixing #19424
| License       | MIT
| Doc PR        | none

There is a confusion between the variable naming, and the result expected.

When prepend variable is false, the currency symbol must be add after the widget.
When the `money_pattern`starts with `{{`, `prepend` variable must be `false`.

Commits
-------

637a441 Fix the money form type render with Bootstrap3
@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jul 25, 2016

Hum looking at the issue and the time of submission, I may have merged this one to quickly? Is it OK or should we revert?

@Th3Mouk

This comment has been minimized.

Contributor

Th3Mouk commented Jul 25, 2016

@nicolas-grekas no it's OK, the other part of the problem "doesn't exist".

The part of code that i was talking isn't called when intl extension is activated.

@Th3Mouk Th3Mouk deleted the Th3Mouk:fix-moneyformtype-b3 branch Jul 26, 2016

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