MWPW-164660: [3-in-1][Milo] Implement a link converter for CTAs using an iFrame #3564

mirafedas · 2025-01-28T11:34:04Z

To display the content of the 3-in-1 modal, we replace the iFrame link from the checkout-link.xlsx file with a link to the appropriate commerce page that includes the segmentation step for the given offer. This replacement occurs only if the link contains the modal=twp, modal=crm, or modal=d2p parameters.

In the threeInOne.js module, we handle the communication protocol to respond to events emitted by the commerce page rendered inside the iFrame, and render our custom loader.

A delay in loading the commerce page content is expected, as we cannot preload the page displayed within the iFrame (similar to the behavior of the upgrade modal).

Authoring:
For commerce CTAs (created in https://milo.adobe.com/tools/ost):

Checkout link must have the workflowStep=segmentation and modal=true parameters,
e.g. https://milo.adobe.com/tools/ost?osi=L2C9cKHNNDaFtBVB6GVsyNI88RlyimSlzVfkMM2gH4A&type=checkoutUrl&text=free-trial&workflowStep=segmentation&modal=true
In the checkout-link.xlsx file all URLs that should be replaced with the new commerce URL should have the modal parameter set. The value should be the modal type: modal=twp, modal=crm, or modal=d2p.
I added these values in the file and previewed it, but I have not published it yet.

Resolves: MWPW-164660

Test URLs:

aem-code-sync · 2025-01-28T11:34:31Z

	Page	Scores	Audits	Google
📱	/drafts/mirafedas/3in1-test-page?martech=off
🖥️	/drafts/mirafedas/3in1-test-page?martech=off

codecov · 2025-01-28T11:35:36Z

Codecov Report

Attention: Patch coverage is 94.13333% with 22 lines in your changes missing coverage. Please review.

Project coverage is 96.51%. Comparing base (cf4c08b) to head (9eff2aa).

Files with missing lines	Patch %	Lines
libs/features/mas/src/checkout-mixin.js	63.26%	18 Missing ⚠️
libs/features/mas/src/buildCheckoutUrl.js	97.67%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            stage    #3564      +/-   ##
==========================================
- Coverage   96.53%   96.51%   -0.03%     
==========================================
  Files         274      277       +3     
  Lines       61849    62209     +360     
==========================================
+ Hits        59705    60039     +334     
- Misses       2144     2170      +26

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

libs/blocks/merch/threeInOne.js

yesil · 2025-01-28T12:53:39Z

libs/blocks/merch/threeInOne.js

+    loading: 'lazy',
+    class: 'loading',
+  });
+  const pCircle = createTag('sp-progress-circle', { label: 'progress circle', indeterminate: true, size: 'l' });


I think if the loading doesn't finish in certain amount of time, we should kill the spinner and show some kind of error message.
to be checked with Nick.

test/blocks/merch/merch.test.js

libs/blocks/merch/threeInOne.js

github-actions · 2025-01-29T01:26:37Z

This pull request is not passing all required checks. Please see this discussion for information on how to get all checks passing. Inconsistent checks can be manually retried. If a test absolutely can not pass for a good reason, please add a comment with an explanation to the PR.

libs/blocks/merch/threeInOne.js

libs/features/mas/src/external.js

libs/features/mas/src/buildCheckoutUrl.js

3ch023

small request to remove checkout builder package from dependencies.
rest looks good to me

narcis-radu · 2025-02-03T12:03:25Z

libs/blocks/merch/merch.js

@@ -496,7 +497,13 @@ export async function openModal(e, url, offerType, hash, extraOptions) {
    const fragmentPath = url.split(/(hlx|aem).(page|live)/).pop();
    modal = await openFragmentModal(fragmentPath, getModal);
  } else {
-    modal = await openExternalModal(url, getModal, extraOptions);
+    const isThreeInOneModal = [MODAL_TYPE_3_IN_1.CRM, MODAL_TYPE_3_IN_1.D2P, MODAL_TYPE_3_IN_1.TWP].includes(el?.getAttribute('data-modal-type')) && el?.href;


If we add additional modal types we would need to maintain the code in two places. Why not use Object.values to create an array from the values?

narcis-radu · 2025-02-03T12:07:51Z

libs/blocks/merch/threeInOne.js

+      document.querySelector('.three-in-one iframe')?.classList?.remove('loading');
+      document.querySelector('.three-in-one sp-theme')?.remove();
+      break;
+    case MSG_SUBTYPE.EXTERNAL:


I see the same action being executed for MSG_SUBTYPE.EXTERNAL, MSG_SUBTYPE.SWITCH, MSG_SUBTYPE.RETURN_BACK. Maybe the code can be refactored a bit here.

narcis-radu · 2025-02-03T12:11:38Z

libs/blocks/merch/merch.js

@@ -496,7 +497,13 @@ export async function openModal(e, url, offerType, hash, extraOptions) {
    const fragmentPath = url.split(/(hlx|aem).(page|live)/).pop();
    modal = await openFragmentModal(fragmentPath, getModal);
  } else {
-    modal = await openExternalModal(url, getModal, extraOptions);
+    const isThreeInOneModal = [MODAL_TYPE_3_IN_1.CRM, MODAL_TYPE_3_IN_1.D2P, MODAL_TYPE_3_IN_1.TWP].includes(el?.getAttribute('data-modal-type')) && el?.href;


The constants are part of merch, but the code looks like it could be any type of modal, without a direct connection to merch. We should either a) make this more explicit, b) make the modal types more general so they can be used outside of merch scope.

The 'trial with purchase' (twp), 'direct to purchase' (d2p), and 'content rich modal' are the only three types of modals we have on catalog and plans pages, and all three are directly connected to merch context, because they are opened by clicking the checkout CTAs. I'm not sure we should make is more general, because this logic is relevant only for these three types of modals.

narcis-radu · 2025-02-03T12:25:37Z

libs/blocks/modal/modal.css

@@ -359,6 +386,13 @@
    width: 80%;
  }

+  .dialog-modal.three-in-one {


I see .dialog-modal.upgrade-flow-modal has the exact properties, can we combine them somehow?

libs/blocks/merch/threeInOne.js

narcis-radu · 2025-02-03T14:23:53Z

libs/blocks/merch/merch.css

+.three-in-one sp-theme {
+  width: max-content;
+  height: max-content;
+  left: 50%; 


Would this handle RTL accordingly?

Thanks, corrected 👍

changes have been implemented

narcis-radu · 2025-02-05T17:34:04Z

libs/blocks/merch/merch.js

@@ -489,14 +490,28 @@ export async function openModal(e, url, offerType, hash, extraOptions) {
    const prevHash = window.location.hash.replace('#', '') === hash ? '' : window.location.hash;
    window.location.hash = hash;
    window.addEventListener('milo:modal:closed', () => {
-      window.history.pushState({}, document.title, `#${prevHash}`);
+      if (prevHash === '') {


Why not do something like

window.history.pushState({}, document.title, prevHash !== '' ? `#${prevHash}` : `${window.location.pathname}${window.location.search}`)

Axelcureno · 2025-02-06T00:18:37Z

libs/blocks/merch/merch.js

-    modal = await openExternalModal(url, getModal, extraOptions);
+    const isThreeInOneModal = Object.values(MODAL_TYPE_3_IN_1).includes(el?.getAttribute('data-modal-type')) && el?.href;
+    if (isThreeInOneModal) {
+      const { default: openThreeInOneModal } = await import('./threeInOne.js');


Camel case for file naming seems odd. Please instead use kebab case.

libs/blocks/merch/three-in-one.js

yesil

I wonder whether we should be using Milo modal to display the 3-in-1 or use SWC to render the modal from MAS directly.
Thus, the integration will be re-usable outside adobe.com.
@mirafedas I don't know how much work is this, especially given the timelines.
this is just to start the reflection on it.
cc: @npeltier @3ch023

yesil · 2025-02-12T16:19:03Z

libs/blocks/merch/constants.js

@@ -0,0 +1,7 @@
+export const MODAL_TYPE_3_IN_1 = {


This constant is duplicated at https://github.com/adobecom/milo/pull/3564/files#diff-e91eb0b5a655ea856ec40025c1071fb88ac9908264c11f760b6208130e1099f0R106.

Wherever it is used, it should import it from mas feature.

to avoid confusion, I recommend to remove this file.

yesil · 2025-02-12T16:21:27Z

libs/blocks/merch/merch.js

    }, { once: true });
  }
  if (isInternalModal(url)) {
    const fragmentPath = url.split(/(hlx|aem).(page|live)/).pop();
    modal = await openFragmentModal(fragmentPath, getModal);
  } else {
-    modal = await openExternalModal(url, getModal, extraOptions);
+    const isThreeInOneModal = Object.values(MODAL_TYPE_3_IN_1).includes(el?.getAttribute('data-modal-type')) && el?.href;


this logic could be a helper function in MAS checkout-mixin some related files.

yesil · 2025-02-12T16:23:25Z

libs/blocks/merch/three-in-one.js

+
+export async function createContent(iframeUrl, modalType) {
+  const { base } = getConfig();
+  await Promise.all([


these imports can go to top of the file.
this is blocking the loading for no reason IMO.

yesil · 2025-02-12T16:25:47Z

libs/blocks/merch/three-in-one.js

+  const pCircle = createTag('sp-progress-circle', { label: 'progress circle', indeterminate: true, size: 'l' });
+  const theme = createTag('sp-theme', { theme: 'spectrum', color: 'light', scale: 'medium', dir: 'ltr' });
+  theme.append(pCircle);
+  content.append(theme);


dark theme support?
also since the markup being generated static, you could also do:

content.innerHTML = '<sp-theme ....>...</sp-theme>';

I rewrote it to use .innerHTML, and regarding the dark theme - if support is needed, I can work on it in the scope of the follow-up tickets, because this has to be aligned with the commerce team, in the current task it is not mentioned, and it would be good to avoid adding more changes to this PR to get it merged before the RCP

yesil · 2025-02-12T16:27:03Z

libs/blocks/merch/three-in-one.js

+  const modalType = el?.getAttribute('data-modal-type');
+  if (!modalType || !iframeUrl) return undefined;
+
+  window.addEventListener('message', handle3in1IFrameEvents);


might we be registering more events that intended if the user closes / re-opens the modal?

I would register the even handler in the module scope, and ignore the messages if the modal is not open.

yesil · 2025-02-12T16:27:55Z

libs/blocks/modal/modal.css

    padding-bottom: 0;
    height: 100%;
  }

-  .dialog-modal.commerce-frame.height-fit-content, .dialog-modal.dynamic-height.height-fit-content {
+  .dialog-modal.commerce-frame.height-fit-content,
+  .dialog-modal.dynamic-height.height-fit-content {


if this is only code formatting, I would omit these changes from your PR.

libs/features/mas/src/checkout-mixin.js

yesil · 2025-02-12T16:31:38Z

libs/features/mas/src/checkout-mixin.js

+                return modalParam;
+            }
+          } catch (error) {
+            // do nothing


log using this.masElement.log

yesil · 2025-02-12T16:32:06Z

libs/features/mas/src/external.js

@@ -1,9 +1,4 @@
 // This file aliases and re-exports commonly used external dependencies
-import {


mirafedas · 2025-02-14T14:53:44Z

@yesil thank you for reviewing, I addressed all comments. Regarding the comment about rendering the modal as SWC directly from MAS - I would wait for the commerce team to develop their web component (as they mentioned at the meeting)

libs/blocks/merch/three-in-one.js

+  content.innerHTML = `<sp-theme system="light" color="light" scale="medium" dir="ltr">
+  <sp-progress-circle label="progress circle" indeterminate="" size="l" dir="ltr" role="progressbar" aria-label="progress circle"></sp-progress-circle>
+  </sp-theme>
+  <iframe src="${iframeUrl}" title="${modalType === MODAL_TYPE_3_IN_1.CRM ? 'Single App' : modalType}" frameborder="0" marginwidth="0" marginheight="0" allowfullscreen="true" loading="lazy" class="loading" style="height: 100%;"></iframe>`;


Axelcureno · 2025-02-18T17:23:14Z

libs/features/mas/build.mjs

@@ -38,6 +38,19 @@ await build({
    outfile: `${outfolder}/mas.js`,
 });

+// constants.js
+await build({


why not part of mas.js tho? @mirafedas @npeltier @3ch023 @yesil

Axelcureno

LGTM, just one question and one recommendation, both trivial.

Axelcureno · 2025-02-18T17:28:26Z

libs/features/mas/src/checkout-mixin.js

+          } catch (error) {
+            this.masElement.log?.error('Failed to add 3-in-1 modal parameters', error);
+          }
+        }


Should we add a catch() here?

There is already a catch statement here :)

Roycethan · 2025-02-21T01:02:02Z

@mirafedas Plz review below and also update the branch with latest to resolve conflicts:

Business tab when clicked in below link
https://mwpw-164660-3-in-1-modal--milo--mirafedas.hlx.page/drafts/mirafedas/3in1-test-page?martech=off#mini-plans-web-cta-photoshop-card
directly navigated to https://commerce.adobe.com/store/email?items%5B0%5D%5Bid%5D=DAC628518DDD843D4D19A8BF7D8FF1FE&co=US&lang=en&cli=adobe_com in a new tab

Expectation as per in 3 in 1 page: it stays in the same page :
https://commerce.adobe.com/store/segmentation?ms=COM&ot=TRIAL&pa=phsp_direct_individual&cli=adobe_com&ctx=if&co=US&lang=en&af=uc_new_user_iframe%2Cuc_new_system_close&ss=segmentation&items%5B0%5D%5Bid%5D=7C2BA37CAA6F683E829D5412A0BA982E&fps=f&ooid=E27CB5D79014ACAB6953B091CEA72228
( we can find this while clicking Business tab in https://commerce.adobe.com/store/segmentation?ms=COM&ot=TRIAL&pa=phsp_direct_individual&cli=adobe_com&ctx=if&co=US&lang=en&af=uc_new_user_iframe%2Cuc_new_system_close&ss=segmentation&items%5B0%5D%5Bid%5D=7C2BA37CAA6F683E829D5412A0BA982E&fps=f&ooid=E27CB5D79014ACAB6953B091CEA72228 )

mirafedas requested a review from a team as a code owner January 28, 2025 11:34

github-advanced-security bot found potential problems Jan 28, 2025

View reviewed changes

libs/blocks/merch/threeInOne.js Fixed Show fixed Hide fixed

libs/blocks/merch/threeInOne.js Fixed Show fixed Hide fixed

libs/blocks/merch/threeInOne.js Fixed Show fixed Hide fixed

mirafedas force-pushed the MWPW-164660-3-in-1-modal branch from 40e1011 to b4bf8e9 Compare January 28, 2025 12:37

yesil reviewed Jan 28, 2025

View reviewed changes

github-advanced-security bot found potential problems Jan 30, 2025

View reviewed changes

libs/blocks/merch/threeInOne.js Fixed Show fixed Hide fixed

3ch023 reviewed Jan 31, 2025

View reviewed changes

libs/features/mas/src/external.js Outdated Show resolved Hide resolved

3ch023 reviewed Jan 31, 2025

View reviewed changes

libs/features/mas/src/buildCheckoutUrl.js Show resolved Hide resolved

3ch023 requested changes Jan 31, 2025

View reviewed changes

3ch023 approved these changes Feb 3, 2025

View reviewed changes

narcis-radu previously requested changes Feb 3, 2025

View reviewed changes

github-advanced-security bot found potential problems Feb 3, 2025

View reviewed changes

libs/blocks/merch/threeInOne.js Fixed Show fixed Hide fixed

narcis-radu reviewed Feb 3, 2025

View reviewed changes

narcis-radu reviewed Feb 5, 2025

View reviewed changes

Axelcureno requested changes Feb 6, 2025

View reviewed changes

github-advanced-security bot found potential problems Feb 6, 2025

View reviewed changes

libs/blocks/merch/three-in-one.js Fixed Show fixed Hide fixed

mirafedas requested review from Axelcureno, narcis-radu and yesil February 7, 2025 14:55

yesil requested changes Feb 12, 2025

View reviewed changes

added 3-in-1 modal

600c6e9

mirafedas force-pushed the MWPW-164660-3-in-1-modal branch from be4444b to 600c6e9 Compare February 13, 2025 16:59

mirafedas requested review from salonijain3, bandana147, sharmrj, nishantka and Deva309 as code owners February 13, 2025 16:59

mirafedas requested a review from sonawanesnehal3 as a code owner February 13, 2025 16:59

mirafedas added 2 commits February 13, 2025 18:04

moved event listener

4d543cd

updated tests

7851171

addressed comments

7c75f2e

github-advanced-security bot found potential problems Feb 14, 2025

View reviewed changes

fixed unit test

88e4f55

Axelcureno reviewed Feb 18, 2025

View reviewed changes

Axelcureno approved these changes Feb 18, 2025

View reviewed changes

mirafedas added 2 commits February 19, 2025 11:37

Merge branch 'stage' into MWPW-164660-3-in-1-modal

a6f737f

moved constants.js into mas.js

571bf64

yesil approved these changes Feb 19, 2025

View reviewed changes

Merge branch 'stage' into MWPW-164660-3-in-1-modal

a9add31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MWPW-164660: [3-in-1][Milo] Implement a link converter for CTAs using an iFrame #3564

MWPW-164660: [3-in-1][Milo] Implement a link converter for CTAs using an iFrame #3564

mirafedas commented Jan 28, 2025 •

edited

Loading

aem-code-sync bot commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

yesil Jan 28, 2025

github-actions bot commented Jan 29, 2025

3ch023 left a comment

narcis-radu Feb 3, 2025

narcis-radu Feb 3, 2025

narcis-radu Feb 3, 2025

mirafedas Feb 4, 2025

narcis-radu Feb 3, 2025

narcis-radu Feb 3, 2025

mirafedas Feb 4, 2025

narcis-radu Feb 5, 2025

Axelcureno Feb 6, 2025

yesil left a comment

yesil Feb 12, 2025

yesil Feb 12, 2025

yesil Feb 12, 2025

yesil Feb 12, 2025

yesil Feb 12, 2025

mirafedas Feb 14, 2025

yesil Feb 12, 2025

yesil Feb 12, 2025

yesil Feb 12, 2025

yesil Feb 12, 2025

mirafedas commented Feb 14, 2025

Axelcureno Feb 18, 2025

Axelcureno left a comment

Axelcureno Feb 18, 2025

mirafedas Feb 19, 2025

Roycethan commented Feb 21, 2025 •

edited

Loading

		@@ -1,9 +1,4 @@
		// This file aliases and re-exports commonly used external dependencies
		import {

MWPW-164660: [3-in-1][Milo] Implement a link converter for CTAs using an iFrame #3564

Are you sure you want to change the base?

MWPW-164660: [3-in-1][Milo] Implement a link converter for CTAs using an iFrame #3564

Conversation

mirafedas commented Jan 28, 2025 • edited Loading

aem-code-sync bot commented Jan 28, 2025 • edited Loading

codecov bot commented Jan 28, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2025

3ch023 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yesil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirafedas commented Feb 14, 2025

Choose a reason for hiding this comment

Axelcureno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roycethan commented Feb 21, 2025 • edited Loading

mirafedas commented Jan 28, 2025 •

edited

Loading

aem-code-sync bot commented Jan 28, 2025 •

edited

Loading

codecov bot commented Jan 28, 2025 •

edited

Loading

Roycethan commented Feb 21, 2025 •

edited

Loading