-
-
Notifications
You must be signed in to change notification settings - Fork 817
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
dev/translation#90 address translation issue on membership #31299
Conversation
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/translation/-/issues/90 |
@@ -116,13 +116,9 @@ | |||
{if !empty($auto_renew)} {* Auto-renew membership confirmation *} | |||
{crmRegion name="contribution-thankyou-recur-membership"} | |||
<br /> | |||
{if !$installments || $installments > 1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only thought here @eileenmcnaughton I wonder is if $installments is always being assigned to the template or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seamuslee001 yeah - I'm confused - I'm thinking it maybe is not - but note that if it is NOT assigned there is still text rendered - it's just that there is
<strong>{ts 1=$frequency_unit}This membership will be renewed automatically every %1.{/ts}</strong>
vs the per-unit version
& it seems like the per unit version is generally preferred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also a problem with
<strong>{ts 1=$frequency_interval 2=$frequency_unit}This membership will be renewed automatically every %1 %2(s).{/ts}</strong>
that seems to be pushed out - I don't know whether you can use the plural whatsit & have day translate to days in English - that seems like it has been left out of scope over a few iterations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I read this right, the every %1 %2(s)
that is still there only affects when a limit is set on the number of instalments, which presumably is not used much.
Ideally it should use the same pattern as for unlimited instalments, with the if/elseif for day/week/month/year (but that's outside the scope of this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mlutfy I pushed in an addition fix for that - if it looks good I suggest we merge this to the rc but only back port the original patch to 5.78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eileenmcnaughton Looks good to me, thanks! I added merge-ready based on the feedback from Samuele on Gitlab (he had also worked on the initial fixes around this).
The (s) pattern doesn't work for translation
Overview
A brief description of the pull request. Keep technical jargon to a minimum. Hyperlink relevant discussions.
Before
What is the old user-interface or technical-contract (as appropriate)?
For optimal clarity, include a concrete example such as a screenshot, GIF (LICEcap, SilentCast), or code-snippet.
After
What changed? What is new old user-interface or technical-contract?
For optimal clarity, include a concrete example such as a screenshot, GIF (LICEcap, SilentCast), or code-snippet.
Technical Details
If the PR involves technical details/changes/considerations which would not be manifest to a casual developer skimming the above sections, please describe the details here.
Comments
Anything else you would like the reviewer to note