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

copy update for update amount SP form (with old min amount) #1352

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

rBangay
Copy link
Contributor

@rBangay rBangay commented Jul 2, 2024

What does this change?

If you have a supporter plus at an old price that is below the new minumum, the update amount form should reflect that in the copy re the new minimum amount you can change it to

Images

Before After

…numum, the update amount form should reflect that in the copy re the new minimum amount you can change it to
@rBangay rBangay requested a review from a team as a code owner July 2, 2024 15:12
Copy link
Member

@tjmw tjmw left a comment

Choose a reason for hiding this comment

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

Looks good 👍 I left a few questions, just to help my understanding really!

@@ -304,6 +312,7 @@ export const SupporterPlusUpdateAmountForm = (
>
<TextInput
label={otherAmountLabel}
data-cy="supporter-plus-other-amount-input"
Copy link
Member

Choose a reason for hiding this comment

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

We can't target this using the label? (Feels a bit closer to how a human would interact with this).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah good shout, I aggree ... the new selector in the test for the input is a bit more involved but I guess the benefit is that we're not polluting the dom and we're closer to how a user would navigate the site

{monthlyOrAnnual.toLowerCase()} amount below{' '}
{minPriceDisplay} please call us via the{' '}
If you would like to{' '}
{currentAmountIsBelowNewMin
Copy link
Member

Choose a reason for hiding this comment

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

Would change work in both cases here? Only ask because it would simplify the code a bit!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah possibly .. I took a steer from the stakeholders re the copy here:
Screenshot 2024-07-03 at 11 35 20

@@ -98,6 +101,10 @@ describe('Update contribution amount', () => {

cy.findByText('Change amount').click();

cy.contains(
/£\d{2,3} per month is the minimum payment to receive this subscription./,
Copy link
Member

Choose a reason for hiding this comment

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

Are we fuzzy matching the amount here to account for future config changes? (I.e. so this test doesn't have to be updated each time the config changes?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I fuzzy matched here to reduce the number of places that you would have to update if/when we do another price rise ... there are still places where you have to do it manually though, so this doesn't solve that 'problem' everywhere, only here.

@rBangay rBangay merged commit 1429b00 into main Jul 3, 2024
13 checks passed
@rBangay rBangay deleted the sp-update-amount-validation branch July 3, 2024 12:20
@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @rBangay 10 minutes and 31 seconds ago) Please check your changes!

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

Successfully merging this pull request may close these issues.

3 participants