Skip to content

[TwigBridge] Amend MoneyType twig to include a space #48335

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

Merged
merged 1 commit into from
Dec 1, 2022

Conversation

mogilvie
Copy link
Contributor

Q A
Branch? 6.3
Bug fix? yes
New feature?
Deprecations?
Tickets
License MIT
Doc PR

Fix for MoneyType not rendering classes in Bootstrap 4&5.

@mogilvie
Copy link
Contributor Author

Failing on:

Unsilenced deprecation notices (1)

  1x: Legacy nullable type detected, please update your code as
         you are using nullable types in a docblock. support will be removed in v2.0.0
    1x in ObjectNormalizerTest::testDoesntHaveIssuesWithUnionConstTypes from Symfony\Component\Serializer\Tests\Normalizer

@stof
Copy link
Member

stof commented Nov 25, 2022

As this is a bugfix, it should probably not target the 6.3 branch

@mogilvie
Copy link
Contributor Author

They failing test looks like something with the ObjectNormalizer @dunglas. I won't even pretend to understand where the 'legacy nullable' type is.

@mogilvie mogilvie changed the base branch from 6.3 to 6.0 November 25, 2022 18:17
@mogilvie mogilvie changed the base branch from 6.0 to 6.3 November 25, 2022 18:18
@mogilvie
Copy link
Contributor Author

mogilvie commented Nov 25, 2022

Argh, fecked that up. I'm checking out for tonight. My apologies to everyone.

@carsonbot carsonbot changed the title Amend MoneyType twig to include a space. [TwigBridge] Amend MoneyType twig to include a space. Nov 25, 2022
@derrabus derrabus modified the milestones: 6.3, 5.4 Nov 25, 2022
@javiereguiluz
Copy link
Member

I haven't looked into the details of this ... but Bootstrap classes for money uses the input-group-text CSS class See https://getbootstrap.com/docs/5.2/forms/input-group/#multiple-addons

So, the existing code (<div class="input-group{{ group_class|default('') }}">) could be correct if group_class is -text ... and the new code (<div class="input-group {{ group_class|default('') }}">) only would be correct if group_class is input-group-text

@mogilvie
Copy link
Contributor Author

mogilvie commented Dec 1, 2022

I haven't looked into the details of this ... but Bootstrap classes for money uses the input-group-text CSS class See https://getbootstrap.com/docs/5.2/forms/input-group/#multiple-addons

So, the existing code (<div class="input-group{{ group_class|default('') }}">) could be correct if group_class is -text ... and the new code (<div class="input-group {{ group_class|default('') }}">) only would be correct if group_class is input-group-text

The inner span is input-group-text. The outer div wrapper is class input-group. The issue is the outer div.
image

@OskarStark OskarStark changed the title [TwigBridge] Amend MoneyType twig to include a space. [TwigBridge] Amend MoneyType twig to include a space Dec 1, 2022
@javiereguiluz
Copy link
Member

Mark, thanks for the additional insights!

@fabpot fabpot changed the base branch from 6.3 to 5.4 December 1, 2022 21:09
@fabpot
Copy link
Member

fabpot commented Dec 1, 2022

Thank you @mogilvie.

@fabpot fabpot merged commit 6c02af9 into symfony:5.4 Dec 1, 2022
@fabpot fabpot mentioned this pull request Dec 6, 2022
This was referenced Dec 28, 2022
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.

6 participants