MWPW-140481: Delayed modal #1877

mirafedas · 2024-02-14T13:36:05Z

This feature enables the delayed display of a modal, primarily intended for promotional purposes.

When parsing the promotional manifest, if we encounter a rule labeled insertContentAfter containing a link with valid parameters for delay and display, we dynamically generate a hidden link within the specified selector. This link behaves conventionally, triggering a modal upon click. Following this setup, we introduce a delay of some number of seconds before unveiling the modal, simultaneously appending a modal hash to the browser's URL. If the modal is configured for a one-time session display (display="session"), we remember this in sessionStorage to prevent subsequent displays upon page reload.

Resolves: MWPW-140481

Authoring documentation

Test URLs:

Before: not applicable, because this is a newly added feature
After:
First page
Second page
Feel free to change the display param to either pageload or session in this file

aem-code-sync · 2024-02-14T13:36:31Z

Page	Scores	Audits	Google
/drafts/mirafedas/showdelayedmodalhere?manifest=%2Ffragments%2Fmirafedas%2Fpromo s%2F2024%2Fglobal%2Fdelayed-modal-promo%2Fdelayed-modal-promo.json--all
/drafts/mirafedas/secondpage

yesil · 2024-02-14T14:23:07Z

libs/features/personalization/personalization.js

@@ -99,18 +99,105 @@ export const preloadManifests = ({ targetManifests = [], persManifests = [] }) =

 export const getFileName = (path) => path?.split('/').pop();

-const createFrag = (el, url, manifestId) => {
-  let href = url;
+export async function initDelayedModal({


I think modal logic should go in modal.js

yesil · 2024-02-14T14:25:33Z

libs/features/personalization/personalization.js

+    if (!window.sessionStorage.getItem('wasDelayedModalShown')) {
+      setTimeout(() => {
+        getModal(details);
+        window.sessionStorage.setItem('wasDelayedModalShown', true);


this implementation supports one single delayed modal.
if the user navigates from one page to another in the same session, I think the other modals will not be shown.

@yesil
I created two test pages to verify this:

First page

Second page

On both pages I placed some traditional modals.
To me all works as expected.
If you want to test yourself, feel free to change the display param to either pageload or session in this file

this test doesn't cover oncePerSession option.
please use oncePerSession, and link one page to the other one and try to navigate.
I expect the second modal will not be shown.

@yesil
I cross-linked both pages, set the display param to session, and it works as expected:

When I open the first page for the first time, the modal is shown, and the the wasDelayedModalShown key is saved in the session storage;

Then I click the link to navigate to the second page, and the modal is not shown, which is expected as the modal should be shown once per browser session and it was already shown on the first page.

Other modals which I linked to both pages get opened on click.
I will ask Reuben to verify this as well.
I also refactored the code a bit to make it simpler, could you please review once again?

yesil · 2024-02-14T14:27:21Z

test/features/personalization/delayedModal.test.js

+});
+
+it('creates and opens the delayed modal', async () => {
+  const hash = '#delayed-modal';


delayed-modal shounds to technical.

I wonder if we could adopt a more short/cryptic naming here, like md or dm.

auniverseaway

I think this can be simpler.

codecov · 2024-02-15T10:07:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.68%. Comparing base (f5e68ff) to head (c11d4e8).
Report is 1 commits behind head on main.

❗ Current head c11d4e8 differs from pull request most recent head 2725066. Consider uploading reports for the commit 2725066 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1877      +/-   ##
==========================================
+ Coverage   95.64%   95.68%   +0.04%     
==========================================
  Files         159      159              
  Lines       41183    41345     +162     
==========================================
+ Hits        39388    39562     +174     
+ Misses       1795     1783      -12

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

mirafedas · 2024-02-15T12:41:29Z

@auniverseaway
Do you have any tips on how to simplify this? Otherwise, I'm not sure what changes I should make

chrischrischris

I haven't had a chance for an in-depth review but wanted to ask - why are all the changes in personalization needed?

Also we don't want personalization.js to load any external files (modal.js) unless absolutely needed.

I think that all of the modal logic needs to be in modal.js and should not require MEP in any way.

chrischrischris · 2024-02-20T17:54:03Z

My thoughts:

No modal code should be in personalization.js, all functionality should be in modal.js
Instead of query params, we should have the params built into the modal name: #modal-name:delay=3:display=session.
- This allows for the modal.js code to remove the params from the url without needing to do a refresh.
delayed modals should work without MEP - e.g: www.adobe.com/nz/mypage#modal-name:delay=3:display=session will work on page load.

With these changes, there are no changes needed to MEP. The fragment link set in the manifest would contain the info and modal.js would handle everything else.

More minor thoughts:

What should the behavior be if multiple fragments specify a delayed modal?
What happens when a modal is deep linked (so displaying immediately on page load) and a fragment has a delayed modal?
sessionstorage should differentiate between which delayed modal was shown. A single wasDelayedModalShown key would stop a possible other delayed modal unrelated to the original one from being show. Something like shown:modal-name.

Implementation details:

The parsing of the modal hash happens in modal.js init. Your code should go there to check / parse / setTimeout the opening of a modal hash with delay params.
There is also a modal:open event that you can fire that will trigger a modal if that approach works better.

Roycethan · 2024-02-20T20:33:49Z

@mirafedas few observations adding here: plz review

I don't see modal loading on successive page loads, Say below link (with hash) is deep-linked in some other site and user clik on the CTA to launch this deep link with hash
https://mwpw-140481-delayed-modal--milo--mirafedas.hlx.page/drafts/mirafedas/secondpage#delayed-modal
OR
User launch https://mwpw-140481-delayed-modal--milo--mirafedas.hlx.page/drafts/mirafedas/secondpage and click on say 'Save' to navigate to Checkout (if linked to open in same page ) and the click Back in browser

2 Click event outside the modal has web interaction name 'modalClose:buttonClose'
Ideally it should be : 'modalClose:overlayClose' to distinguish between button click and outside clicks.

…das/milo into MWPW-140481-delayed-modal

mirafedas · 2024-02-28T14:05:50Z

Closing this PR as these changes are not actual anymore, and opening a new one

mirafedas requested a review from a team as a code owner February 14, 2024 13:36

mirafedas added @modal Run Modal Tests Nala new-feature New block or other feature needs-verification PR requires E2E testing by a reviewer run-nala Run Nala Test Automation against PR labels Feb 14, 2024

mirafedas requested review from yesil, Axelcureno, honstar, 3ch023, vladen, bozojovicic and VKniaz February 14, 2024 14:12

yesil reviewed Feb 14, 2024

View reviewed changes

auniverseaway requested changes Feb 14, 2024

View reviewed changes

mirafedas added the do not merge PR should not be merged yet label Feb 15, 2024

mirafedas force-pushed the MWPW-140481-delayed-modal branch from 46631ba to e4dfe62 Compare February 15, 2024 13:45

mirafedas removed the do not merge PR should not be merged yet label Feb 15, 2024

mirafedas force-pushed the MWPW-140481-delayed-modal branch from eebf905 to 80e56ec Compare February 19, 2024 15:04

added a possibility to render a modal with delay

c11d4e8

mirafedas force-pushed the MWPW-140481-delayed-modal branch from c0c5740 to c11d4e8 Compare February 19, 2024 16:11

chrischrischris requested changes Feb 19, 2024

View reviewed changes

mirafedas added 4 commits February 22, 2024 11:58

Merge branch 'MWPW-140481-delayed-modal' of https://github.com/mirafe…

3c13b0e

…das/milo into MWPW-140481-delayed-modal

Merge branch 'main' into MWPW-140481-delayed-modal

bc8ad2a

new version: modal is shown on event, logic moved partly to utils

6496e78

Merge branch 'main' into MWPW-140481-delayed-modal

f232da7

mirafedas added 5 commits February 23, 2024 11:27

added analytics event on delayed modal opening

8cadf8a

changed session storage identifier

c8042f6

Merge branch 'main' into MWPW-140481-delayed-modal

8a7c65d

added unit tests for new utils functions

3d398a5

saving current state before rewriting all again :)

2725066

mirafedas closed this Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MWPW-140481: Delayed modal #1877

MWPW-140481: Delayed modal #1877

mirafedas commented Feb 14, 2024 •

edited

Loading

aem-code-sync bot commented Feb 14, 2024 •

edited

Loading

yesil Feb 14, 2024

yesil Feb 14, 2024

mirafedas Feb 15, 2024 •

edited

Loading

yesil Feb 16, 2024

mirafedas Feb 19, 2024 •

edited

Loading

yesil Feb 14, 2024

auniverseaway left a comment

codecov bot commented Feb 15, 2024 •

edited

Loading

mirafedas commented Feb 15, 2024

chrischrischris left a comment •

edited

Loading

chrischrischris commented Feb 20, 2024 •

edited

Loading

Roycethan commented Feb 20, 2024 •

edited

Loading

mirafedas commented Feb 28, 2024

MWPW-140481: Delayed modal #1877

MWPW-140481: Delayed modal #1877

Conversation

mirafedas commented Feb 14, 2024 • edited Loading

aem-code-sync bot commented Feb 14, 2024 • edited Loading

yesil Feb 14, 2024

Choose a reason for hiding this comment

yesil Feb 14, 2024

Choose a reason for hiding this comment

mirafedas Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

yesil Feb 16, 2024

Choose a reason for hiding this comment

mirafedas Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

yesil Feb 14, 2024

Choose a reason for hiding this comment

auniverseaway left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 15, 2024 • edited Loading

Codecov Report

mirafedas commented Feb 15, 2024

chrischrischris left a comment • edited Loading

Choose a reason for hiding this comment

chrischrischris commented Feb 20, 2024 • edited Loading

Roycethan commented Feb 20, 2024 • edited Loading

mirafedas commented Feb 28, 2024

mirafedas commented Feb 14, 2024 •

edited

Loading

aem-code-sync bot commented Feb 14, 2024 •

edited

Loading

mirafedas Feb 15, 2024 •

edited

Loading

mirafedas Feb 19, 2024 •

edited

Loading

codecov bot commented Feb 15, 2024 •

edited

Loading

chrischrischris left a comment •

edited

Loading

chrischrischris commented Feb 20, 2024 •

edited

Loading

Roycethan commented Feb 20, 2024 •

edited

Loading