MWPW-151513: Search results vanished when user clicks on Marquee CTA:Start free trail #2406
Conversation
mirafedas
requested review from
yesil,
Axelcureno,
3ch023,
bozojovicic and
seanchoi-dev
|
@mirafedas thanks for the fix, can you add a unit test? |
libs/blocks/modal/modal.js
Outdated
| @@ -245,5 +250,8 @@ window.addEventListener('hashchange', (e) => { | |||
| } else { | |||
| const details = findDetails(window.location.hash, null); | |||
| if (details) getModal(details); | |||
| if (e.oldURL?.includes('#search=')) { | |||
There was a problem hiding this comment.
Milo should not be opinionated about the deep link values. So I don't expect to see search=.
Also, when I change the deeplink values by clicking on the category filters, they are not captured.
There was a problem hiding this comment.
@yesil thank you, I added a unit test and changed the condition, so now all filters get captured
|
@mirafedas this is good for a generic use case, but when we click on a modal link, the as the deeplink is lost, the page updates. cc: @Roycethan @afmicka |
|
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
force-pushed
the
MWPW-151513-search-results
branch
2 times, most recently
from
5e85fb0 to
9a48bce
Compare
|
@yesil I added tacocat change in this PR: https://git.corp.adobe.com/wcms/tacocat.js/pull/615 , and included it in this PR |
|
mirafedas
force-pushed
the
MWPW-151513-search-results
branch
from
a10931d to
636b39b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2406 +/- ##
=======================================
Coverage 95.82% 95.82%
=======================================
Files 175 175
Lines 45985 45993 +8
=======================================
+ Hits 44064 44074 +10
+ Misses 1921 1919 -2 ☔ View full report in Codecov by Sentry. |
|
@mirafedas the tacocat PR for merch-card-collection has been merged, please rebuild it from the develop branch and update here. |
|
@yesil thank you, I updated this PR, it's ready for review :) |
|
@mirafedas you also need to ship sidenav in CC so that the search text doesn't vanish when you click the modal link. |
|
@yesil I have a pending CC PR for updating sidenav, it's in the scope of a different task, but it will ship the latest sidenav to CC: adobecom/cc#335 |
|
This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label. |
| @@ -65,6 +66,10 @@ export function closeModal(modal) { | |||
| const hashId = window.location.hash.replace('#', ''); | |||
| if (hashId === modal.id) window.history.pushState('', document.title, `${window.location.pathname}${window.location.search}`); | |||
| isDelayedModal = false; | |||
| if (prevHash) { | |||
There was a problem hiding this comment.
How does this work? If prevHash is defined as empty string in L15, wouldn't this if always pass as true?
There was a problem hiding this comment.
We set the prevHash value on line 263 in the event listener which gets executed on the hash change. So when the closeModal function gets called, the prevHash may have a value
afmicka
added
run-nala
|
Skipped merging 2406: MWPW-151513: Search results vanished when user clicks on Marquee CTA:Start free trail due to insufficient approvals. Required: 2 approvals |
* stage: Mwpw-142267: Merch What's Included and Merch Mnemonic List (TwP) (adobecom#2554) MWPW-149124 Improve Focus Page for Performance Improvement Tiger Team (adobecom#2391) Correctly send the created PR slacks (adobecom#2566) MWPW-153167: caas-config change to enable hiding date detail information (adobecom#2553) MWPW-152016 - Localization target preview for transcreation (adobecom#2564) Relax CORS restrictions for module imports (adobecom#2549) MWPW-153600 [PEP] loader bar on PEP prompt is seen loaded Left to right in RTL locale (adobecom#2548) MWPW-146962 [milo] Text link behaving like button in FAQ section (adobecom#2530) MWPW-152280 MEP: Only preload fragments that are in the 1st section (adobecom#2525) MWPW-152697 Fix Marketo mobile horizontal scroll (adobecom#2514) MWPW-151513: Search results vanished when user clicks on Marquee CTA:Start free trail (adobecom#2406) MWPW-154013 PEP prompt redirection is broken in stage after the PEP dismissal PR merge (adobecom#2547) MWPW-153962: Introduce maslibs query parameter (adobecom#2544) Central georouting support (adobecom#2531) [MWPW-152278] Avoid empty CSS requests (adobecom#2524) MWPW-152918 Fix Marketo button font (adobecom#2513) # Conflicts: # libs/deps/merch-card.js
When we search for some cards on the catalog page, for example, 'acrobat', the hash
#search=acrobatgets added to the URL, and analogically for other filter params. When we open a modal in the marquee, we overwrite the hash value, and when we close the modal, the hash gets removed, so we lose the search results. To avoid this, before opening the modal, we remember the previous hash and restore it after it gets closed.Related tacocat PR: https://git.corp.adobe.com/wcms/tacocat.js/pull/615 (the change from it is already included in /deps in this PR).
Resolves: MWPW-151513
Test URLs:
CC:
Milo: