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

dev/core#1613 Change event registration button text based on if there are additional participants #20251

Merged

Conversation

larssandergreen
Copy link
Contributor

@larssandergreen larssandergreen commented May 8, 2021

Overview

Submit buttons label for event registrations can be confusing. Discussion here. This PR simplifies the button label and switches it depending on user selection of additional participants.

Before

If additional participants are allowed or confirmation is required for an event registration, the button to go to the next page is labelled "Review your registration". This is not correct in the case where the user selected additional participants, as the next page is the additional participants page, not the review page.

After

If additional participants are allowed or confirmation is required, the button to go to the next page is simply labelled "Review". If the user selects additional participants, the button text is changed to "Continue". This is consistent with the additional participants page, where the button to go to the next page is labelled "Continue" as well.

Comments

This works, but it does seem a little hacky especially when it comes to translations. If this is just a bad idea, I'm happy to be told that!
See also the pending PR that this is added on top of.
Changes one translation string.

@civibot
Copy link

civibot bot commented May 8, 2021

(Standard links)

@civibot civibot bot added the master label May 8, 2021
Comment on lines 170 to 192
function additionalParticipantsChange() {
toggleAdditionalParticipants();
allowParticipant();
}

function toggleAdditionalParticipants() {
var submit_button = cj("#crm-submit-buttons > button").html();
{/literal}
var review_translated = '{ts}Review{/ts}';
var continue_translated = '{ts}Continue{/ts}';
{literal}
if (cj('#additional_participants').val()) {
cj("#additionalParticipantsDescription").show();
cj("#crm-submit-buttons > button").html(submit_button.replace(review_translated, continue_translated));
} else {
cj("#additionalParticipantsDescription").hide();
cj("#crm-submit-buttons > button").html(submit_button.replace(continue_translated, review_translated));
}
}

window.onload = function() {
toggleAdditionalParticipants();
};
Copy link
Member

@colemanw colemanw May 27, 2021

Choose a reason for hiding this comment

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

Not your fault, but the javascript style in this .tpl file is horribly outdated. You don't have to clean up the existing code (although if you feel inspired to do so, by all means open another PR for that) but please use the new style for new code.

  • Enclose your code with CRM.$(function($) {...}); to avoid creating global functions.
  • Use $ instead of the deprecated cj within the closure.
  • Keep the whole thing literal, only break out of literal within an actual string and use escaping, e.g. `var review_translated = '{/literal}{ts escape="js"}Review{/ts}{literal}';
  • You don't need to use window.onload as the closure automatically waits for $(document).ready()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better late than never here, but this is now done. Thanks.

@colemanw
Copy link
Member

Thanks for this PR. I think it's great to solve this problem.
Once your changes are finished, please rebase and squash into a single commit.

@larssandergreen larssandergreen force-pushed the change-registration-button-text branch 5 times, most recently from ef1208e to 5ba4725 Compare August 2, 2021 23:07
@in2part
Copy link

in2part commented Aug 3, 2021

@larssandergreen Can you please make a small fix in the text inside the description div?
From "Fill in your registration information on this page. You will be able to enter THEIR registration information..."
To "Fill in your registration information on this page. You will be able to enter THE registration information..."

…l participants

cleanup

fix typo in description
@larssandergreen larssandergreen force-pushed the change-registration-button-text branch from 5ba4725 to f347129 Compare August 3, 2021 14:15
@larssandergreen
Copy link
Contributor Author

@in2part Thanks for the catch, fixed.

@BettyDolfing
Copy link

test this please

@jaapjansma
Copy link
Contributor

  • General standards
    • Explain (r-explain)
      • PASS : The goal/problem/solution have been adequately explained in the PR.
    • User impact (r-user)
      • PASS: The change would be intuitive or unnoticeable for a majority of users who work with this feature.
    • Documentation (r-doc)
      • PASS: The changes do not require documentation.
    • Run it (r-run)
      • PASS: We tested it by creating an event with multiple registration and we registered for one person and we did a registration for two participants. Both ok.
  • Developer standards
    • Technical impact (r-tech)
      • PASS: The change preserves compatibility with existing callers/code/downstream.
    • Code quality (r-code)
      • PASS: The functionality, purpose, and style of the code seems clear+sensible.
    • Maintainability (r-maint)
      • PASS: The change is trivial enough that it does not require tests.
    • Test results (r-test)
      • PASS: The test results are all-clear.

@colemanw or @totten or @eileenmcnaughton can one of you merge this PR? We think it is ready to be merged. Btw. the review request has also been resolved.

@colemanw colemanw merged commit 4080069 into civicrm:master Jan 24, 2022
@larssandergreen larssandergreen deleted the change-registration-button-text branch November 5, 2022 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants