Skip to content
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-155694 Dispatch sticky close event for dynamic chat #2788

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

Copy link
Contributor

aem-code-sync bot commented Aug 26, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link

codecov bot commented Aug 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.91%. Comparing base (d90dcc0) to head (dccfc85).
Report is 37 commits behind head on stage.

Additional details and impacted files
@@           Coverage Diff           @@
##            stage    #2788   +/-   ##
=======================================
  Coverage   95.90%   95.91%           
=======================================
  Files         173      173           
  Lines       45842    45848    +6     
=======================================
+ Hits        43967    43974    +7     
+ Misses       1875     1874    -1     

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

@@ -201,7 +201,7 @@

.section.sticky-bottom.promo-sticky-section {
background: none;
z-index: 4;
z-index: 40000;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing the other martech element has a high z-index like this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the dynamic chat is at 10k.

Copy link
Contributor

@meganthecoder meganthecoder left a comment

Choose a reason for hiding this comment

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

I think this is fine given what currently exists, but @ryanmparrish @elan-tbx we might want to rethink some of the sticky functionality. Didn't our tiger team say that any sticky section should be closable? It seems like maybe the close button/functionality should go with the section metadata code rather than the block code, if possible.

@elan-tbx
Copy link
Contributor

elan-tbx commented Sep 3, 2024

@meganthecoder Looks like that's already a part of Asides, I can create a due-diligence ticket for this to be added to Notifications as well.

It's just tricky, because the actual DOM element of a close button would need to live within each block itself, so having the section inject a close button into the block feels wrong.

Would make sense to me if we dispatched the same event onClick across those blocks and let section-metadata handle the rest, though.

@meganthecoder
Copy link
Contributor

@meganthecoder Looks like that's already a part of Asides, I can create a due-diligence ticket for this to be added to Notifications as well.

It's just tricky, because the actual DOM element of a close button would need to live within each block itself, so having the section inject a close button into the block feels wrong.

Would make sense to me if we dispatched the same event onClick across those blocks and let section-metadata handle the rest, though.

I don't think there's a need for another ticket. Brandon covered notifications as well as asides in this PR. I was more thinking about the fact that other blocks can use the sticky section metadata functionality, and we'd need to add this code to every one. However, if you think that the close button needs to be in the block code then that sounds good.

Thanks!

Copy link
Contributor

github-actions bot commented Sep 4, 2024

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

@Dli3 Dli3 added verified PR has been E2E tested by a reviewer Ready for Stage labels Sep 4, 2024
@milo-pr-merge milo-pr-merge bot merged commit 5a7baab into stage Sep 5, 2024
20 checks passed
@milo-pr-merge milo-pr-merge bot deleted the bmarshal/chat-promo branch September 5, 2024 16:45
@milo-pr-merge milo-pr-merge bot mentioned this pull request Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

5 participants