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): ensure variable modal added to ras overlays #1861

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

chickenn00dle
Copy link
Contributor

@chickenn00dle chickenn00dle commented Aug 29, 2024

All Submissions:

Changes proposed in this Pull Request:

Closes https://app.asana.com/0/1207817176293825/1207922674964414/f

This PR adds variation modals to ras overlays to ensure there aren't any overlapping popups:

Screenshot 2024-08-29 at 10 54 41

Note: I accidentally pushed these changes directly to epic/ras-acc, but have reverted in 2f16c97 🤦

How to test the changes in this Pull Request:

  1. Set up a delayed overlay campaign for any segment of user
  2. On a post or page the campaign targets add a checkout button for a variable product, allowing the reader to select variation
  3. As a reader targeted by the campaign, visit the page or post and click the checkout button to bring up the variation modal. Don't make a selection.
  4. On epic/ras-acc, the prompt will appear below the variation modal as pictured above. On this branch it wont.

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?

Copy link
Contributor

@laurelfulford laurelfulford 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! You beat me to the feedback about this with the Newspack Popups PR AND beat me to fixing the close button on the variation modal 😆

@chickenn00dle
Copy link
Contributor Author

Thanks again @laurelfulford!

You beat me to the feedback about this with the Newspack Popups PR AND beat me to fixing the close button on the variation modal

Sorry! I should have communicated the changes better. Hopefully I didn't waste too much of your time 🙏

@chickenn00dle chickenn00dle merged commit 5556943 into epic/ras-acc Aug 29, 2024
10 checks passed
@chickenn00dle chickenn00dle deleted the fix/add-variable-modal-to-ras-overlays branch August 29, 2024 18:48
@laurelfulford
Copy link
Contributor

No worries! 😄 Both were small, and I would've figured it out if I had skimmed all of the PRs up for review before I dove in!

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