[TwigBridge][Form] Fix hidden currency element with Bootstrap 3 theme #25233
Conversation
| [@type="text"] | ||
| [@name="name"] | ||
| [@class="my&class form-control"] | ||
| [@value="1234.56"] |
There was a problem hiding this comment.
does this prevent regression also?
There was a problem hiding this comment.
What kind of regression do you mean?
There was a problem hiding this comment.
if a input-group-addon is rendered again, this xpath would still match no?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added more predicates.
| {% if not append %} | ||
| <span class="input-group-addon">{{ money_pattern|replace({ '{{ widget }}':''}) }}</span> | ||
| {% endif %} | ||
| {% set append_before = not (money_pattern starts with '{{') %} |
There was a problem hiding this comment.
what about a check like money_pattern is '{{ widget }}' wouldnt that be more concise?
There was a problem hiding this comment.
Don't know, a starts with or ends with test would still be required in case the currency symbol must be appended.
There was a problem hiding this comment.
Right, lets call those variables append + prepend?
c1e8ca0 to
31f3a4f
Compare
31f3a4f to
c5af7fd
Compare
|
Tests added. |
|
Do we have the same issue with Bootstrap 4 support? |
|
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. |
|
Thank you @julienfalque. |
…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
When using a
MoneyTypefield, one can passcurrency=falseoption 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.