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.

Sign up for GitHub

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

Jump to bottom

feature(global-cta-modal-implementation): Global CTA Video Modal. #335

Merged
merged 7 commits into from
Sep 15, 2023
Merged

feature(global-cta-modal-implementation): Global CTA Video Modal. #335

merged 7 commits into from
Sep 15, 2023

Conversation

nimithshetty17
Copy link

@nimithshetty17 nimithshetty17 commented Sep 12, 2023 •
edited by proeung
Loading

Issue

Fixes #MERATIVE-858

Description

We need to ensure that all of the button styles (e.g.. Primary, Secondary, Tertiary) (see - https://main--merative2--hlxsites.hlx.page/block-library/buttons/buttons-video) can support video modal when the href is defined as a video type.

Changed

image

Test URLs

Testing Instruction

To test if the video player is opening a modal for all the button and link styles.

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 12, 2023

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 12, 2023

Page Scores Audits Google
/block-library/buttons/buttons-video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@proeung proeung added the Needs design QA PRs on feature requests and new components have to get design approval before merge. label Sep 12, 2023
@aem-code-sync
Copy link

aem-code-sync bot commented Sep 13, 2023

Page Scores Audits Google
/block-library/buttons/buttons-video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 13, 2023

Page Scores Audits Google
/block-library/buttons/buttons-video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

sharathmrft
sharathmrft approved these changes Sep 13, 2023
View reviewed changes
Copy link

@sharathmrft sharathmrft left a comment

Choose a reason for hiding this comment

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

@nimithshetty17
The video player is opening a modal for all the button and link styles.

@proeung @sachinmesh

sachinmesh
sachinmesh approved these changes Sep 13, 2023
View reviewed changes
Copy link

@sachinmesh sachinmesh left a comment

Choose a reason for hiding this comment

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

All CTAs are opening the modal.

@sachinmesh sachinmesh added Ready to merge Label for the pull requests that are ready to merge and removed Needs design QA PRs on feature requests and new components have to get design approval before merge. labels Sep 13, 2023
proeung
proeung suggested changes Sep 13, 2023
View reviewed changes
Copy link

@proeung proeung left a comment

Choose a reason for hiding this comment

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

@nimithshetty17 Looks good overall! I just have minor code suggestion, but the modal overlay is rendering when the a link is set to YT or MP4 paths. https://global-cta-modal-implementation--merative2--nimithshetty17.hlx.page/block-library/buttons/buttons-all

Screenshot 2023-09-13 at 2 16 49 PM

blocks/linkable-cards/linkable-cards.js Outdated Show resolved Hide resolved
scripts/lib-franklin.js Outdated Show resolved Hide resolved
@proeung proeung added 🛠️ fix needed and removed Ready to merge Label for the pull requests that are ready to merge labels Sep 13, 2023
@aem-code-sync
Copy link

aem-code-sync bot commented Sep 14, 2023

Page Scores Audits Google
/block-library/buttons/buttons-all PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

proeung
proeung approved these changes Sep 14, 2023
View reviewed changes
Copy link

@proeung proeung left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for addressing my code suggestions.

@proeung proeung added Ready to merge Label for the pull requests that are ready to merge and removed 🛠️ fix needed labels Sep 14, 2023
@aem-code-sync
Copy link

aem-code-sync bot commented Sep 14, 2023

Page Scores Audits Google
/block-library/buttons/buttons-all PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Sep 15, 2023

Page Scores Audits Google
/block-library/buttons/buttons-all PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@proeung proeung merged commit 0d0ccc6 into hlxsites:main Sep 15, 2023
2 checks passed
proeung added a commit that referenced this pull request Sep 15, 2023
@proeung
Revert "feature(global-cta-modal-implementation): Global CTA Video Mo…
1dcd758 
…dal. (#335)"

This reverts commit 0d0ccc6.
@proeung
Copy link

proeung commented Sep 15, 2023

@nimithshetty17 I decided to revert this PR from the main branch due to some of the 404 errors in the console log (see attached). Looks like the video model gets added to every block element (https://github.com/hlxsites/merative2/pull/335/files#diff-a5ee87ff9063f921ba01933977e6164e5729551986030f0ed7eb747bcffb740fR727), which tells the the loadBlock function to build and load in the blocks (https://github.com/hlxsites/merative2/blob/main/scripts/lib-franklin.js#L349) for classes .video-modal-overlay and .video-modal-container.

You might need to adjust your approach to appending the video overlay outside of the main HTML tag and append the video embed onClick.

Screenshot 2023-09-15 at 1 06 41 PM

Let's open a new PR for this work and include that fix that eliminates the 404 error in the console log.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sharathmrft sharathmrft sharathmrft approved these changes

@sachinmesh sachinmesh sachinmesh approved these changes

@proeung proeung proeung approved these changes

@keith-kaplan keith-kaplan Awaiting requested review from keith-kaplan

@sahmad-merative sahmad-merative Awaiting requested review from sahmad-merative

Assignees
No one assigned
Labels
Ready to merge Label for the pull requests that are ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nimithshetty17 @proeung @sachinmesh @sharathmrft