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

Format money in custom fields once, on the tpl layer #22728

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Format money in custom fields once, on the tpl layer

Further to #22727 - this attempts to address the double formatting issue

Before

Formatted in the php layer AND the tpl layer

After

Only formatted in the tpl layer

Technical Details

This ensures the raw value is available to the template layer and uses it
rather than the formatted value for money display.

It rather feels it would be better to separate out this view use case in case
there are others hidden in there - but I think this tpl is only
used with this assign

Comments

This ensures the raw value is available to the template layer and uses it
rather than the formatted value for money display.

It rather feels it would be better to separate out this view use case in case
there are others hidden in there - but I think this tpl is only
used with this assign
@civibot
Copy link

civibot bot commented Feb 8, 2022

(Standard links)

@demeritcowboy
Copy link
Contributor

This looks ok. It's strange it's not on all forms but this is civi.

Money dropdowns are a bit broken but that's pre-existing.

I have some out of scope comments I'll just add to the lab ticket.

@demeritcowboy demeritcowboy added the merge ready PR will be merged after a few days if there are no objections label Feb 9, 2022
@yashodha
Copy link
Contributor

@eileenmcnaughton @demeritcowboy looks good, merging this. Thanks!

@yashodha yashodha merged commit bbdc7c8 into civicrm:master Feb 11, 2022
@eileenmcnaughton eileenmcnaughton deleted the local branch February 11, 2022 04:35
@eileenmcnaughton
Copy link
Contributor Author

thanks @yashodha @demeritcowboy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants