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][Form] Fix hidden currency element with Bootstrap 3 theme #25233

Merged
merged 1 commit into from Dec 11, 2017

Conversation

Projects
None yet
5 participants
@julienfalque
Copy link
Contributor

commented Nov 30, 2017

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

When using a MoneyType field, one can pass currency=false option to hide the currency symbol. This does not work well when using the Bootstrap 3 theme: the symbol is not displayed but HTML elements that are supposed to contain it are still rendered.

@ro0NL

ro0NL approved these changes Dec 1, 2017

Copy link
Contributor

left a comment

👍 conflicts with #25167

[@type="text"]
[@name="name"]
[@class="my&class form-control"]
[@value="1234.56"]

This comment has been minimized.

Copy link
@ro0NL

ro0NL Dec 1, 2017

Contributor

does this prevent regression also?

This comment has been minimized.

Copy link
@julienfalque

julienfalque Dec 1, 2017

Author Contributor

What kind of regression do you mean?

This comment has been minimized.

Copy link
@ro0NL

ro0NL Dec 1, 2017

Contributor

if a input-group-addon is rendered again, this xpath would still match no?

This comment has been minimized.

Copy link
@julienfalque

julienfalque Dec 1, 2017

Author Contributor

Indeed, the test still passes if any extra element is rendered, as long as the <input> is at first nesting level. But this actually applies to most of these tests with XPath. This could (should?) probably be improved but in a separate PR IMO. WDYT?

This comment has been minimized.

Copy link
@ro0NL

ro0NL Dec 1, 2017

Contributor

Yeah, but now we just test form_widget_simple again; which is probably already covered in some other test. For this new test i'd expect it to cover the change; no rendering of input addons in case of currency=false.

Or at least not containing a currency symbol in general.

This comment has been minimized.

Copy link
@julienfalque

julienfalque Dec 2, 2017

Author Contributor

Added more predicates.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Dec 2, 2017

Contributor

Nice!

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

This comment has been minimized.

Copy link
@ro0NL

ro0NL Dec 1, 2017

Contributor

what about a check like money_pattern is '{{ widget }}' wouldnt that be more concise?

This comment has been minimized.

Copy link
@julienfalque

julienfalque Dec 1, 2017

Author Contributor

Don't know, a starts with or ends with test would still be required in case the currency symbol must be appended.

This comment has been minimized.

Copy link
@ro0NL

ro0NL Dec 1, 2017

Contributor

Right, lets call those variables append + prepend?

This comment has been minimized.

Copy link
@julienfalque

julienfalque Dec 1, 2017

Author Contributor

Done.

@julienfalque julienfalque force-pushed the julienfalque:bootstrap-3-money branch 2 times, most recently from c1e8ca0 to 31f3a4f Dec 1, 2017

@ro0NL

ro0NL approved these changes Dec 2, 2017

Copy link
Contributor

left a comment

Nice catch. Perhaps, to close the gates, add 2 more tests in MoneyTypeTest

 $this->assertSame('{{ widget }}', $view->vars['money_pattern']); // currency=false

 $this->assertSame('... {{ widget }}', $view->vars['money_pattern']); // prepended currency

@julienfalque julienfalque force-pushed the julienfalque:bootstrap-3-money branch from 31f3a4f to c5af7fd Dec 2, 2017

@julienfalque

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2017

Tests added.

@fabpot

This comment has been minimized.

Copy link
Member

commented Dec 2, 2017

Do we have the same issue with Bootstrap 4 support?

@julienfalque

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2017

It looks like when Bootstrap 4 theme was added, most of the content of the Bootstrap 3 theme was moved to a common file used by both so yes, I think Bootstrap 4 theme has the same issue. Merging this PR up to 3.4 would probably bring conflicts: the patch must be moved to the common file, which will also fix the issue for Bootstrap 4. Same applies with the tests as they use a common base class.

@fabpot

fabpot approved these changes Dec 11, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

Thank you @julienfalque.

@fabpot fabpot merged commit c5af7fd into symfony:2.7 Dec 11, 2017

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

fabpot added a commit that referenced this pull request Dec 11, 2017

bug #25233 [TwigBridge][Form] Fix hidden currency element with Bootst…
…rap 3 theme (julienfalque)

This PR was merged into the 2.7 branch.

Discussion
----------

[TwigBridge][Form] Fix hidden currency element with Bootstrap 3 theme

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

When using a `MoneyType` field, one can pass `currency=false` option to hide the currency symbol. This does not work well when using the Bootstrap 3 theme: the symbol is not displayed but HTML elements that are supposed to contain it are still rendered.

Commits
-------

c5af7fd Fix hidden currency element with Bootstrap 3 theme

@julienfalque julienfalque deleted the julienfalque:bootstrap-3-money branch Dec 11, 2017

This was referenced Dec 15, 2017

This was referenced Jan 5, 2018

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