MWPW-181371: mweb floating cta changes fixes #5150
Conversation
hkhatana26
requested review from
drashti1712,
hkuraware,
nkthakur48 and
suhjainadobe
and removed request for
a team
|
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. |
|
Please provide after branch url to test , do not see with branch mwpw-181371-mweb-floatingcta--milo--hkhatana26 page loading |
|
Hi @hkhatana26 , |
hkhatana26
force-pushed
the
mwpw-181371-mweb-floatingcta
branch
from
844d5b5 to
e7fea76
Compare
nkthakur48
changed the title
|
Hi Harshad , fix given is not just changing the flow of floating cta , but impacting the existing promo banners as well .Please take a look |
|
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. |
| || entry.boundingClientRect.top < 0 | ||
| || stickySectionEl?.getBoundingClientRect().y > 0; | ||
| el.classList.toggle('hide-sticky-section', shouldHideSticky); | ||
| } else if (target.matches('merch-card')) { |
There was a problem hiding this comment.
Minor: The nested if-else structure in the intersection observer is becoming difficult to follow
Could you see if extracting the merch-card handling into a separate function makes sense?
There was a problem hiding this comment.
I have extracted the merch card handling and moved the related condition into a separate function.
| }); | ||
| }); | ||
| }); | ||
| mutationObserver.observe(document.body, { childList: true, subtree: true }); |
There was a problem hiding this comment.
Observing document.body with subtree: true monitors ALL DOM changes across the entire page. Could you check whether scoping the observer to a more specific container or using event delegation makes sense here?
There was a problem hiding this comment.
Both observers are needed throughout the lifecycle of page .The IntersectionObserver must remain active to react to scroll-based visibility changes throughout the page lifecycle. The MutationObserver cannot be safely disconnected because merch cards load asynchronously and there is no reliable signal indicating when all cards have been rendered.
| io.observe(stickySectionEl); | ||
| } | ||
|
|
||
| const mutationObserver = new MutationObserver((mutations) => { |
There was a problem hiding this comment.
The MutationObserver lacks cleanup, which can lead to potential memory leaks. Could you return the cleanup functions and call them appropriately
return () => {
mutationObserver.disconnect();
io.disconnect();
};There was a problem hiding this comment.
i am observing main tag because merch card will be loaded into that.
zagi25
left a comment
zagi25
left a comment
There was a problem hiding this comment.
It looks like that this solution only covers merch-cards, but based on the ticket description, any section should be able to trigger 'hide-action'.
| }); | ||
| }); | ||
| }); | ||
| mutationObserver.observe(main, { childList: true, subtree: true }); |
There was a problem hiding this comment.
I don't think observing main tag mutations is a good idea, this can get very expensive and it can lead to performance issues.
There was a problem hiding this comment.
I have removed the MutationObserver on main.
|
|
||
| if (target === document.querySelector('footer')) { | ||
| el.classList.toggle('fill-sticky-section', isIntersecting); | ||
| } else if (target === document.querySelector('.hide-at-intersection')) { |
There was a problem hiding this comment.
Instead of using Mutation Observer on main, is it possible to expand this functionality or to implement something similar to mark section for which notification should be hidden ?
There was a problem hiding this comment.
I have removed the MutationObserver on main and made this behavior more explicit. Sections that need to hide the notification are now marked using hide-at-intersection-start / hide-at-intersection-end and handled via IntersectionObserver.
This avoids expensive DOM observation and lets any section trigger the hide-action in a cleaner, more scalable way.
|
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. |
4 participants
Fixed the floating CTA issue and hide it for the merch card section.
Resolves: [MWPW-181371]
Test URLs:
QA: https://main--cc--adobecom.aem.page/drafts/harshad/floating-cta?milolibs=mwpw-181371-mweb-floatingcta--milo--hkhatana26