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

Improve formatting for recurring contributions in user dashboard #25058

Closed

Conversation

webmaster-cses-org-uk
Copy link
Contributor

@webmaster-cses-org-uk webmaster-cses-org-uk commented Nov 26, 2022

Overview

Fixes a few minor issues in the formatting of recurring contribution information on the user dashboard. Intended to close https://lab.civicrm.org/dev/core/-/issues/4005.

Before

Some specific and minor issues with the display of recurring contributions on the user dashboard:

  1. No handling for if (installments == 0), i.e. the recurring contribution is open-ended. Just leaves a blank string which is confusing to users.
  2. No spacing between clauses in 'Terms' - makes difficult to read. Also untranslated strings.
  3. Unnecessary colon after 'Terms' in table header (doesn't match other header cells).

Screenshot below shows issues.

image
image

After

The above issues resolved.

  1. "for X instalments" omitted from 'Terms' and "/X" omitted from instalments counter if open-ended (X is blank).
  2. spacing corrected by use of {ts}...{/ts} in Smarty template.
  3. colon removed from table header cell.

Screenshot below shows example.

image

Technical Details

Minor tweaks to Smarty file to implement these improvements.

Comments

Tested on CiviCRM 5.55.2.

@civibot
Copy link

civibot bot commented Nov 26, 2022

(Standard links)

@civibot civibot bot added the master label Nov 26, 2022
@webmaster-cses-org-uk
Copy link
Contributor Author

OK strange one - got a failed test but nothing to do with the proposed change.

E2E\Core\AssetBuilderTest::testInvalid
GuzzleHttp\Exception\ConnectException: cURL error 28: Operation timed out after 2001 milliseconds with 0 bytes received (see https://curl.haxx.se/libcurl/c/libcurl-errors.html)

Seems to be related to an issue spotted prevously in a19ebbf.

Looks like an intermittent issue. Can't see how to force a rebuild?

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@demeritcowboy
Copy link
Contributor

I think I agree with everything here except I'm not sure the meaning of - is clearer. Do you think 1/∞ is too geeky? I suppose it could be a word like {ts}Unlimited{/ts}.

Also can you squash commits?

@webmaster-cses-org-uk
Copy link
Contributor Author

Hi @demeritcowboy thanks for your feedback.

Yes I too deliberated over this somewhat (and arrived at the same conclusion about 1/∞). I'd be happy with "unlimited" or similar - very open to suggestions here.

Let's allow a little longer to see what others think, then I'll make any final changes and squash commits.

@webmaster-cses-org-uk
Copy link
Contributor Author

@demeritcowboy Further to the above comments, I have removed the "/..." completely for open-ended contributions in the same vein as for the "Terms" column. So now an unlimited contribution simply shows the number of payments:

Before:
image

After:
image

I have been unable to squash commits. GitHub online doesn't seem to have a facility for this (?) so I tried desktop, but it required me to first sync with the master, then said I can't squash commits because there is a merge commit in the history. I will contact GitHub support.

@demeritcowboy
Copy link
Contributor

Yeah I'm not familiar with github desktop - you might need command line. An alternative is:

  1. Create a new branch in your fork by reapplying all your changes to master.
  2. Create a new PR and just link back to this one.
  3. Close this PR.
  4. Your old changes will still be available online as part of this PR.

- "for X instalments" omitted from 'Terms' and "/X" omitted from instalments counter if open-ended (X is blank).
- Spacing corrected by use of {ts}...{/ts} in Smarty template.
- Colon removed from table header cell.
@webmaster-cses-org-uk
Copy link
Contributor Author

OK commits now squashed successfully.

This is thanks to GitHub support, who kindly provided me with the necessary command line procedure to do this!

{ts 1=$recurRows.$id.frequency_interval 2=$recurRows.$id.frequency_unit} every %1 %2{/ts}
{if !empty($recurRows.$id.installments)}
{ts 1=$recurRows.$id.installments} for %1 installments{/ts}
{/if}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice on the rebase.

Just noticing the {ts} - there's been a trend to replace these with something more translation-friendly even though it means more code. When broken up like this and without much context and where part of it is a translatable option value with plurals (e.g. every %1 %2) it's harder to translate. Here's an example:

{if $installments}
{if $frequency_interval > 1}
<p>
<strong>{ts 1=$frequency_interval 2=$frequency_unit 3=$installments}I want to contribute this amount every %1 %2s for %3 installments.{/ts}</strong>
</p>
{else}
<p>
<strong>{ts 1=$frequency_unit 2=$installments}I want to contribute this amount every %1 for %2 installments.{/ts}</strong>
</p>
{/if}
{else}
{if $frequency_interval > 1}
{if $frequency_unit eq 'day'}
<p><strong>{ts 1=$frequency_interval}I want to contribute this amount every %1 days.{/ts}</strong></p>
{elseif $frequency_unit eq 'week'}
<p><strong>{ts 1=$frequency_interval}I want to contribute this amount every %1 weeks.{/ts}</strong></p>
{elseif $frequency_unit eq 'month'}
<p><strong>{ts 1=$frequency_interval}I want to contribute this amount every %1 months.{/ts}</strong></p>
{elseif $frequency_unit eq 'year'}
<p><strong>{ts 1=$frequency_interval}I want to contribute this amount every %1 years.{/ts}</strong></p>
{/if}
{else}
{if $frequency_unit eq 'day'}
<p><strong>{ts}I want to contribute this amount every day.{/ts}</strong></p>
{elseif $frequency_unit eq 'week'}
<p><strong>{ts}I want to contribute this amount every week.{/ts}</strong></p>
{elseif $frequency_unit eq 'month'}
<p><strong>{ts}I want to contribute this amount every month.{/ts}</strong></p>
{elseif $frequency_unit eq 'year'}
<p><strong>{ts}I want to contribute this amount every year.{/ts}</strong></p>
{/if}
{/if}

@civicrm-builder
Copy link

Can one of the admins verify this patch?

@colemanw
Copy link
Member

@webmaster-cses-org-uk thanks for your work on this PR.

Unfortunately it hasn't received attention in nearly a year, and at this point we have a new extension which provides an updated version of the User Dashboard via SearchKit: #27792

If possible, please test out that new extension & give any feedback (or pull-requests, ideally!) to help improve it.

Thanks again.

@colemanw colemanw closed this Oct 17, 2023
@webmaster-cses-org-uk
Copy link
Contributor Author

@colemanw Thank you for the updates. Sounds like this will fix the specific issues and design out a load of other legacy things too :)

Could you please advise where / how I can download this extension? Doesn't appear on the CiviCRM Extensions page in the admin area and can't find it on the CiviCRM extensions website? Currently running 5.66.1.

@colemanw
Copy link
Member

@webmaster-cses-org-uk that extension is actually bundled with CiviCRM itself, but won't be available until 5.68 is released. There will be a beta version available on Nov 1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants