8000 MWPW-175589 and MWPW-175712: Modal does not reopen when navigation back by mirafedas · Pull Request #4485 · adobecom/milo · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

MWPW-175589 and MWPW-175712: Modal does not reopen when navigation back #4485

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

Merged

Conversation

mirafedas
Copy link
Contributor
@mirafedas mirafedas commented Jun 27, 2025

This PR fixes modal functionality for the following scenarios:

  1. Browser navigation with modals: When a user opens a modal, navigates to another page, then clicks the browser's back button (or directly visits a URL with a modal hash), the modal will now properly reopen.

  2. Filter preservation on catalog page (and any page that uses the merch-card-collection with the merch-sidebar):
    When users have applied filters on the catalog page (like https://main--cc--adobecom.aem.live/products/catalog#category=graphic-design&types=desktop), opened the modal and clicked "Back" in the browser- the hash should change to the previous one (it does), and the modal should get closed (this was not working). Also added checking to make sure the modal gets (re)opened only once, even if there are multiple CTAs with the same data-modal-id attribute.

  3. Multiple clicks on the same CTA on a slow network cause multiple modals to open. Now it opens only one.
    Note: Clicking the modal multiple times adds a grey overlay to the commerce page. This issue should be addressed by the commerce team.

Added Nala tests for the first use-case.

Resolves: MWPW-175589 and MWPW-175712

Test URLs:
Before:

After:

@mirafedas mirafedas requested review from a team as code owners June 27, 2025 14:56
Copy link
Contributor
aem-code-sync bot commented Jun 27, 2025

Copy link
Contributor

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.

@mirafedas mirafedas force-pushed the mwpw-174745-modal-redirection branch from 8b207b7 to 891f578 Compare July 2, 2025 13:26
@mirafedas mirafedas changed the title MWPW-175589: Modal does not reopen when navigation back MWPW-175589 and MWPW-175712: Modal does not reopen when navigation back Jul 2, 2025
@mirafedas mirafedas force-pushed the mwpw-174745-modal-redirection branch from dcc45a7 to 191d276 Compare July 7, 2025 08:25
Copy link
Contributor
@yesil yesil left a comment

Choose a reason for hiding this comment

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

nit: comments are unnecessary bytes, maybe we can move them in a merch.md file.

@@ -941,3 +993,15 @@ export default async function init(el) {
log.warn('Failed to get context:', { el });
return null;
}

window.addEventListener('hashchange', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you could do: window.addEventListener('hashchange', updateModalState);

}

/* Use-case #5:
Hash removed from URL.
Copy link
Contributor

Choose a reason for hiding this comment

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

when does this happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When user removes the hash from the URL manually

@3ch023
Copy link
Contributor
3ch023 commented Jul 7, 2025

nit: comments are unnecessary bytes, maybe we can move them in a merch.md file.

i like the idea

@3ch023 3ch023 added commerce high priority Why is this a high priority? Blocker? Critical? Dependency? labels Jul 7, 2025
@afmicka
Copy link
Contributor
afmicka commented Jul 7, 2025

@mirafedas @3ch023 Number 2 does not work properly for me with the changes.
https://main--cc--adobecom.aem.live/products/catalog?milolibs=mwpw-174745-modal-redirection--milo--mirafedas#category=graphic-design

If i go to this page, open the modal, click back in the browser, the page reloads together with the modal and the modal hash. Does not go back to the filtered page. After this, if i try to manually add the url with filter, it still reopens the modal

@mirafedas
Copy link
Contributor Author

@afmicka fixed, could you please check again?

Copy link
Contributor
github-actions bot commented Jul 8, 2025

Reminder to set the Ready for Stage label - to queue this to get merged to stage & production.

@afmicka
Copy link
Contributor
afmicka commented Jul 8, 2025

@mirafedas @3ch023 i see one issue on plans page that on the first load in incognito the url with hash does not open the modal. If the user opens another modal (different product) and closes it, the original from the hash is suddenly being opened. We might want to fix that in a separate PR. Approving this one.

https://main--cc--adobecom.aem.live/creativecloud/plans?milolibs=mwpw-174745-modal-redirection--milo--mirafedas#miniplans-buy-all-apps

@afmicka afmicka added verified PR has been E2E tested by a reviewer Ready for Stage labels Jul 8, 2025
@milo-pr-merge milo-pr-merge bot merged commit d5c454b into adobecom:stage Jul 8, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commerce high priority Why is this a high priority? Blocker? Critical? Dependency? Ready for Stage verified PR has been E2E tested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0