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: Apply taggedElement to improve iframe height resizing in v3 forms #7607

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

JoshuaHungDinh
Copy link
Contributor

@JoshuaHungDinh JoshuaHungDinh commented Nov 5, 2024

Resolves GIVE-1429

Description

This includes the heightCalculation prop on the Iframe-Resizer-react component for v3 forms. The taggedElement used can be found in the DonationFormViewModel.php class. The element can be found via id="root-givewp-donation-form" . taggedElements are denoted via data-iframe-height

I also noticed the parent page script for the iframe-resizer JS package in embed.ts was enqueued for v3 forms. It is targeting an iframe via iframe[data-givewp-embed] which does not align with the iframe currently used to render v3 forms.

The iframe-resizer-react documents do not explicitly mention to load the parent page script. So I have removed the script from enqueuing. Reference: https://www.npmjs.com/package/iframe-resizer-react/v/1.1.0

Additional Context

  • In an earlier NextGen version the BlockRenderController rendered an iframe with the data attributedata-givewp-embed
    Reference: Pull Request
    File: src/NextGen/DonationForm/Blocks/DonationFormBlock/Controllers/BlockRenderController.php
  • v2 forms have there own implementation of iframe-resizer that is passed via the window object.

Affects

v3 forms on the frontend, including confirmation pages.

Visuals

Testing v3 Forms

testing.v3.forms.mov

Testing v3 Forms as a Block & Shortcode

testing.block.and.shortcodes.mov

Testing v2 Forms

testing-v2-forms.mov

Testing Instructions

  • Create a v3 form to easily view the dynamic changes in height add some simple text fields to the second section.
  • Use the MultiStep forms and navigate the pages, we are testing mainly the previous button functionality to ensure the iframe not only expands but reduces height as well.
  • Ensure all three forms continue to display as expected.
  • Process a donation verify the Confirmation page continues to load in an iframe with the taggedElement.
  • Verify v2 form templates also continue to resize dynamically.

Pre-review Checklist

  • Acceptance criteria satisfied and marked in related issue
  • Relevant @unreleased tags included in DocBlocks
  • Self Review of code and UX completed

@JoshuaHungDinh JoshuaHungDinh marked this pull request as ready for review November 6, 2024 21:12
Comment on lines -107 to -114
(new EnqueueScript(
'givewp-donation-form-embed',
'build/donationFormEmbed.js',
GIVE_PLUGIN_DIR,
GIVE_PLUGIN_URL,
'give'
))->loadInFooter()->enqueue();

Copy link
Contributor

Choose a reason for hiding this comment

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

This script is also used in the confirmation page, let's just make sure there are no side effects there (I believe it's loaded seperately)

Copy link
Contributor Author

@JoshuaHungDinh JoshuaHungDinh Nov 6, 2024

Choose a reason for hiding this comment

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

Yes! The videos up top are actually me just testing via different form rendering. At the very end I inspect the Iframe on the confirmation page & you can see the taggedElement and the iframe rendered which does not hold the data-givewp-embed so it's not being targeted by the file.

@@ -73,6 +73,7 @@ function DonationFormBlockApp({
id={embedId}
src={dataSrc}
checkOrigin={false}
heightCalculationMethod={'taggedElement'}
Copy link
Contributor

Choose a reason for hiding this comment

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

I revisited the original embed file and remembered that I added the iframe resizer to look for the data attribute data-givewp-embed. Is that still at thing?

Copy link
Contributor Author

@JoshuaHungDinh JoshuaHungDinh Nov 6, 2024

Choose a reason for hiding this comment

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

After looking a bit further , I found this is still very much a thing 😄.

embed.ts is required for the Confirmation Page. Without it the v3 confirmation page does not size properly.

In the console I can see the iframe using data-givewp-embed.
Screenshot 2024-11-06 at 2 30 59 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay so if i'm understanding correctly, we don't need the embed script for the donation form because we are using the react version, but the confirmation page does not use that so we still need the vanilla embed script. That sound right?

Copy link
Contributor Author

@JoshuaHungDinh JoshuaHungDinh Nov 7, 2024

Choose a reason for hiding this comment

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

Yes, the confirmation page displayed through the standard form submission process still uses the React version. However, confirmation pages generated through other methods, such as shortcodes, will still require the embed script to function properly.

Copy link
Contributor

@jonwaldstein jonwaldstein left a comment

Choose a reason for hiding this comment

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

@JoshuaHungDinh nice job!

Copy link
Member

@rickalday rickalday left a comment

Choose a reason for hiding this comment

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

Passed manual QA tests.

@JoshuaHungDinh JoshuaHungDinh merged commit 982ead3 into develop Nov 8, 2024
20 checks passed
@JoshuaHungDinh JoshuaHungDinh deleted the fix/v3-iframe-resizer-dynamic-height branch November 8, 2024 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants