-
Notifications
You must be signed in to change notification settings - Fork 53
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
TWFY specific donate page #1777
Conversation
Questions:
|
Simplified approach dumping the fancy box: @zarino: would probably be worth a quick opinion from you at this point in the spirit of "does this pass an uglyness/easyness balance test?" |
@ajparsons Looks fine to me! Tricky things, donation prompts – you want them to be in-your-face enough that people notice them, but tidy enough that people don’t feel like they‘re about to be scammed. This one toes that line pretty well 👍 One thing I’d check is how it looks on narow screens – do the buttons wrap nicely, etc. |
<img role="presentation" src="/images/payment/amex-b933c9009eeaf8cfd07e789c549b8c57.svg" alt="amex" id="amex"> | ||
<img role="presentation" src="/images/payment/mastercard-86e9a2b929496a34918767093c470935.svg" alt="mastercard" id="mastercard"> | ||
<img role="presentation" src="/images/payment/visa-fb36094822f73d7bc581f6c0bad1c201.svg" alt="visa" id="visa"> | ||
<img role="presentation" src="/images/payment/google_pay-ca6cc2f4ee364c7966f8fabf064849fe.svg" alt="GOOGLE_PAY" id="GOOGLE_PAY"> |
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.
Seems odd to have this and not apple pay when stripe supports both? Do we really want to show card mechanisms here?
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.
I'm not that invested in it - it's simpler without - was just experimenting replicating what the stripe box had done previously.
Only thing is being clear what it does take so people don't need to click through to the main page but also... it's fine if they do that.
Re gift aid - what's the issue here? Happy to chat about it. |
@dracos: Several potential gift aid issues:
Which @jaquarone has confirmed we need to be tell HMRC we've told people. I'm trying using a custom field to capture the info in stripe: (know about the typo) Possibly can try squeezing it in on the confirmation page? Open to other ideas. There's a "stop doing the simple thing and do a proper integration with our own form" somewhere around this point - especially where if it ends up being slightly harder to access in stripe backend. |
I dunno about the "simple thing" but given TWFY/mysociety.org are both PHP, copying the existing Stripe code from .org would probably Just Work? It's all self-contained, nothing to do with wordpress or anything like that :) And would then definitely pass through the data in the same way if that's a concern. |
Yeah, the API itself I'm not worried about (as you say just copy the main site, and my understanding is it's a fairly nice one anyway) - it's more having dealing with the formatting for a relatively mini version of the form is getting beyond the "let's just have some easy buttons" that I sold myself on. Just to copy an comment from Jill in here: "make the subscription option £5 a month rather than £10 a year". Ideally if anyone clicked the lighter option it's part of the case for coming back and doing a form that gives a few options. |
036d119
to
8df21db
Compare
Okay, just did the whole thing - copying the mysociety.org stripe form over: I'll tidy up the commits and do a proper PR next week but to flag things I'm hmm about:
Note to self: Change the default utm site bit so we're tracking any uses of this form are TWFY. |
69cd664
to
09d0804
Compare
Ok! So this is ready for review. A support us page with a stripe donate form. Broadly - I have imported the mysociety.org form and stylings, making some adjustments. This includes some updates to the form from https://github.com/mysociety/orgsites/pull/1354 and some extra discoveries while doing this (such as better handling for subscriptions) that I'm going to move back into that in a second. I've assumed the existing stripe credentials can be used - but now need to include reCAPTCHA credentials. Recording.2024-03-18.at.10.40.50.AM.mp4 |
Fixup is just applying Zarino's comments from the other PR here https://github.com/mysociety/orgsites/pull/1354#pullrequestreview-1942708519 |
Note that this probably needs to have separate stripe credentials to the existing ones (API goes to SocietyWorks account). Not a big change so not going to make it until other reviews are in. |
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.
@ajparsons I’ve added a fixup commit that simplifies the styling, markup, and JavaScript. You might want to check it still does all the things you need it to do.
That's great, thanks! I've changed one thing where the error message wasn't being wrapped by your new wrapper. I think the error is a bit less ERROR ERROR now - but still noticeable. |
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 small things :)
markdown/support-us-failed.md
Outdated
@@ -0,0 +1,5 @@ | |||
# Something went wrong with your donation | |||
|
|||
If you’d like to carry on with your donation, [click here](/support-us/) to start again. |
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.
Please don't use click here as link text e.g. https://usability.yale.edu/web-accessibility/articles/links#:~:text=Avoid%20link%20text%20like%20%E2%80%9CClick,experience%20with%20duplicated%20link%20text. - How about, If you'd like to carry on with your donation, [please try again[.
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.
I'll go back and fix the mysociety.org text I copied this form at the same time.
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.
60c67e6 and changed mysociety.org
@@ -1,7 +1,9 @@ | |||
<div class="full-page static-page toc-page"> | |||
<div class="full-page__row"> | |||
<div class="toc-page__col"> | |||
<div class="toc js-toc"></div> | |||
<?php if ($show_menu == true) { ?> |
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.
This can just be if ($show_menu)
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.
<div id="spinner" class="mysoc-spinner mysoc-spinner--small" role="status"> | ||
<span class="sr-only">Processing…</span> | ||
</div> | ||
<p><small>Payment methods avaliable: Card, PayPal, Apple Pay, Google Pay, Direct Debit</small></p> |
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.
Misspelling of available
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.
classes/Renderer/Markdown.php
Outdated
ob_start(); | ||
include($path); | ||
$var=ob_get_contents(); | ||
ob_end_clean(); |
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.
You can do these two lines in one with $var = ob_get_clean();
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.
|
||
$default_amounts = array( | ||
'monthly' => '10', | ||
'annually' => '10', |
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.
So a one-off default to 50, but annual default to only 10, and monthly is also 10? Just the jump seemed a bit odd to me.
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.
This is a bit of an experiment seeing if annual is more popular than monthly, but yes, will have another pass on the amounts.
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.
Changed by 0fd154c
$default_type = 'annually'; | ||
|
||
# use the how-often parameter if set, if not default to option at end of line (options are 'monthly', 'annually', or 'one-off') | ||
$initial_payment_type = isset($_GET['how-often']) ? $_GET['how-often'] : $default_type; |
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.
As we now have PHP 7, we have the null coalescing operator now, so this line can be $initial_payment_type = $_GET['how-often'] ?? $default_type;
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.
Changed by 5229a86
return htmlspecialchars($text, ENT_QUOTES, "UTF-8"); | ||
} | ||
|
||
function get_or_default($name, $default_value = "") |
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.
This function isn't needed in PHP7, as above. Also, you didn't use it some places you could :)
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.
Changed by 5229a86
<p class="donate-form__error"><?=$error ?></p> | ||
<?php } ?> | ||
<noscript> | ||
<p class="donate-form__error">Unfortunately, our payment |
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.
Could we link directly to a payment page on Stripe here, now that they have them, is that possible?
Don't think there was a way back when we did this originally.
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.
Also this text appears in blue on white? .donate-form__error-wrapper sets the colour to blue for some reason?
This applies to all the errors.
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.
In principle we could put a payment link here, but stripe docs wrap that in a JavaScript button to do some captcha link things I think so don't want to proactively. BUT, will add better reporting to the supporters channel of how many people land here.
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.
Blue fixed, the alert colour gets overridden for reasons I don't follow, and just put the red in again directly: 36a39cc
<form class="donate-form" method="post" name="donation_form"> | ||
|
||
<div class="donate-form__error-wrapper"> | ||
<?php if (isset($error) and $error) { ?> |
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.
Don't think $error is ever set by anything?
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.
I think a legacy from the older paypal forms, etc that may have refreshed the page rather than using ajax. Removing.
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.
10da724
to
1788a5c
Compare
Fixed made to above comments - one new commit to redirect existing donate links towards the new page (not important we get them all). |
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.
👍
- Replace previous link to mySociety - Create new markdown-based internal page
- Copies over approach from mysociety.org - Requires new recapacha keys in config - Create new stub pages for thanks/failed in twfy.
This is a work-in-progress PR to add an internal donate page to TWFY.
Basically it's a new markdown-based page to dump some twfy specific language and intergrade a donate box.
I am experimenting with just using stripe's donation links/boxes to speed up time to deploy.
We're basically wanting to test a theory we're losing people by making people go through mySociety.org. If people use these links - we can justify spending a bit more time on a stripe integration that moves the form into the site.
The downside is less customisability. Need to check if the gift-aid approach is compliant or if we'd just want to leave that out while we're testing the approach.
The content of the page itself we can update in this google doc for the moment: https://docs.google.com/document/d/1q_uuD44yG_80SVUVRyBrREHzBwSaPs75f9eu_QJDk1k/edit