-
Notifications
You must be signed in to change notification settings - Fork 190
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
MWPW-175589 and MWPW-175712: Modal does not reopen when navigation back #4485
Conversation
|
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. |
8b207b7
to
891f578
Compare
dcc45a7
to
191d276
Compare
There was a problem hiding this 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.
libs/blocks/merch/merch.js
Outdated
@@ -941,3 +993,15 @@ export default async function init(el) { | |||
log.warn('Failed to get context:', { el }); | |||
return null; | |||
} | |||
|
|||
window.addEventListener('hashchange', () => { |
There was a problem hiding this comment.
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);
libs/blocks/merch/merch.js
Outdated
} | ||
|
||
/* Use-case #5: | ||
Hash removed from URL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when does this happen?
There was a problem hiding this comment.
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
i like the idea |
@mirafedas @3ch023 Number 2 does not work properly for me with the changes. 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 |
@afmicka fixed, could you please check again? |
Reminder to set the |
@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. |
This PR fixes modal functionality for the following scenarios:
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.
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.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: