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(ras-acc): don't show phone field when it's not enabled #1771

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

laurelfulford
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

In #1762, I tried adding an extra class to the phone billing field to help even out the checkout form, without realizing that this made the phone field always show, even when not enabled.

This PR adds a check to make sure the phone field should be showing before adding the CSS class.

How to test the changes in this Pull Request:

  1. On epic/ras-acc, navigate to WP Admin > Newspack > Reader Revenue > Donations, and turn off the 'Phone' field in the billing field settings.
  2. Run a checkout with the modal checkout; note that you get a label-less field at the bottom of the billing details. If you inspect the form, you'll see it's the phone field:

image

  1. Apply this PR.
  2. Repeat step 2; confirm that you don't get the random field at the bottom anymore.
  3. Navigate back to the Reader Revenue > Donation settings, and enable the phone field.
  4. Repeat step 2; confirm that the phone field shows, and that it still has form-row-last CSS class.

image

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?

@laurelfulford
Copy link
Contributor Author

Thanks @adekbadek!

@laurelfulford laurelfulford merged commit 8c480ab into epic/ras-acc Jun 27, 2024
9 checks passed
@laurelfulford laurelfulford deleted the fix/correct-phone-field branch June 27, 2024 18:55
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.

2 participants