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

fix(donate): reset "other" value when switching tiers #1549

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

miguelpeixe
Copy link
Member

All Submissions:

Changes proposed in this Pull Request:

Suppose a reader inputs an invalid value to the "Other" tier option and selects any other tier. In that case, the form will fail to submit due to the browser's native validation for the number input.

This PR implements a reset to the input whenever the tier selection changes, preventing the value from failing form validation.

How to test the changes in this Pull Request:

  1. While in the release branch confirm the issue by visiting a page with a frequency-based donate block
  2. Input a value lower than the allowed minimum (default is $5)
  3. Switch to a suggested value tier and confirm you're not able to continue donating
  4. Switch back to "Other", place a valid number, switch back to the suggested option and confirm you're able to continue
  5. Switch to this branch, repeat steps 2 and 3, and confirm you are able to continue
  6. Close the modal, switch back to "Other" and confirm the value is the default suggestion

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@miguelpeixe miguelpeixe self-assigned this Oct 9, 2023
@miguelpeixe miguelpeixe requested a review from a team as a code owner October 9, 2023 19:26
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

Works for the described scenario, but in testing this I found another related bug that we should fix as well:

In step 3, after inputting the invalid value in "Other", switch to a different frequency tab entirely anad attempt to donate. At this point no matter what value you input the form will throw an error because the invalid input in the other Frequency tier is not focusable:

An invalid form control with name='donation_value_month_other' is not focusable. <input type=​"number" min=​"5" name=​"donation_value_month_other" value=​"25" id=​"newspack-tier-month-52187-other-input">​

@miguelpeixe
Copy link
Member Author

Is this error only being thrown in this branch? I think it's related to the input not being visible when other tiers are selected, not related to this PR.

@dkoo
Copy link
Contributor

dkoo commented Oct 9, 2023

Is this error only being thrown in this branch?

No, the bug is present on release and master as well, but it feels very related to the bug fixed in this PR since it results in a similar reader experience. I think if we were to also reset the "Other" input values whenever switching tabs (not just tiers) it would solve the issue.

@miguelpeixe
Copy link
Member Author

Aaah I see what you mean. I thought the focusable issue was something else. Looking.

@miguelpeixe
Copy link
Member Author

Fixed in 97d5f77

@miguelpeixe miguelpeixe requested a review from dkoo October 9, 2023 21:18
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

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

That fixes it!

@miguelpeixe miguelpeixe merged commit 844505f into release Oct 10, 2023
1 check passed
@miguelpeixe miguelpeixe deleted the hotfix/reset-other-value-on-tier-switch branch October 10, 2023 12:10
matticbot pushed a commit that referenced this pull request Oct 10, 2023
## [1.75.3](v1.75.2...v1.75.3) (2023-10-10)

### Bug Fixes

* **donate:** reset "other" value when switching tiers ([#1549](#1549)) ([844505f](844505f))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.75.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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