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

fix(video): aligned to design & addressed youtube loading issue #307

Merged
merged 9 commits into from
Aug 9, 2023
Merged

fix(video): aligned to design & addressed youtube loading issue #307

merged 9 commits into from
Aug 9, 2023

Conversation

sahmad-merative
Copy link

@sahmad-merative sahmad-merative commented Aug 2, 2023 •
edited
Loading

Multiple Youtube video issue fixes

Issue

UI fixes : fonts, Spacing, and alignments of text and image fixes

  • Add multiple YouTube videos on a page (I have added few videos on https://main--merative2--sahmad-merative.hlx.page/drafts/saad/leadspace-test).
  • Play any one of the videos in a modal (popup).
  • Close the modal before the video finishes playing.
  • Open another video modal that contains a different YouTube video.
  • Notice that the video from the previous modal resumes playing in the background from the point where the first modal was closed.

Fixes # MERATIVE-784
MERATIVE-796

Description

aligned design according to figma
fixed multiple youtube video on a page issue

Design Specs

Test URLs

Testing Instruction

If applicable, please describe the tests that you ran to verify your changes. Provide instructions and link to the hlx deploy preview so that QA and the design team can provide proper testing.

image image image image

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 2, 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 Aug 2, 2023

Page Scores Audits Google
/block-library/blocks/video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/drafts/saad/leadspace-test 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 Aug 2, 2023
@proeung
Copy link

proeung commented Aug 2, 2023

@sahmad-merative Just a couple of notes before I start to review this PR.

  • Please make sure you have the proper title for the PR. Currently, it's set to 784 video promo and it needs to be descriptive and follows the conventional commit message format (eg. fix(video): aligned to design & addressed youtube loading issue
  • Make sure the PR description area has a proper change list.
  • Provide testing instruction for QA (@sharathmrft ) and stakeholders (@keith-kaplan) - See example here - fix(pdf-viewer): add mobile enhancements to PDF Viewer #302 (comment)

@proeung proeung removed the request for review from arumsey August 2, 2023 15:33
@proeung proeung mentioned this pull request Aug 2, 2023
Merged
sharathmrft
sharathmrft suggested changes Aug 3, 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.

@sahmad-merative
1.Video is not playing for Carbon with video on left.

@sharathmrft
Copy link

@sahmad-merative

Background You tube Video player issue is Tested it is working as expected

@proeung

sachinmesh
sachinmesh suggested changes Aug 4, 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.

  1. Eyebrow font style is wrong.
  2. Eyebrow - 32px spacing - Titile - 32px spacing - Bodycopy
  3. The component should be end to end , no padding on left and right side.
  4. Width of this video block should be 744 x 420px.
image

@sahmad-merative @sharathmrft @proeung

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 7, 2023

Page Scores Audits Google
/block-library/blocks/video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/drafts/saad/leadspace-test PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 7, 2023

Page Scores Audits Google
/block-library/blocks/video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/drafts/saad/leadspace-test PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 7, 2023

Page Scores Audits Google
/block-library/blocks/video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/drafts/saad/leadspace-test PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@proeung proeung changed the title 784 video promo ix(video): aligned to design & addressed youtube loading issue Aug 7, 2023
@aem-code-sync
Copy link

aem-code-sync bot commented Aug 7, 2023

Page Scores Audits Google
/block-library/blocks/video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/drafts/saad/leadspace-test PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

proeung
proeung suggested changes Aug 7, 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.

@sahmad-merative Looks good! I just have a comment about the removal of the data-hj-allow-iframe attribute that's needed within the YT iframe code.

Also, @amol-anand or @helms-charity Can you help review this PR? There are a good amount of code changes within the video.js that would be great for you to take a look and make sure we're not missing anything.

blocks/video/video.js
Comment on lines -121 to -122
// Add the data-hj-allow-iframe attribute
iframe.setAttribute('data-hj-allow-iframe', '');
Copy link

Choose a reason for hiding this comment

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

This code should not be removed. We need to add the data-hj-allow-iframe to the YT iframe.

Copy link
Author

Choose a reason for hiding this comment

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

Changes addressed.

@proeung proeung changed the title ix(video): aligned to design & addressed youtube loading issue fix(video): aligned to design & addressed youtube loading issue Aug 7, 2023
@aem-code-sync
Copy link

aem-code-sync bot commented Aug 8, 2023

Page Scores Audits Google
/block-library/blocks/video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/drafts/saad/leadspace-test PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 8, 2023

Page Scores Audits Google
/block-library/blocks/video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/drafts/saad/leadspace-test PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

proeung
proeung approved these changes Aug 8, 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.

@sahmad-merative Thanks for addressing my feedback. Everything looks good from a code perspective.

I'll defer to @sachinmesh and @sharathmrft to verify the design QA and browser testing portions.

sharathmrft
sharathmrft approved these changes Aug 9, 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.

@sahmad-merative You tube player is verified it is working as expected and verified in different browsers as well.
@proeung @Shalini-SB

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 9, 2023

Page Scores Audits Google
/block-library/blocks/video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/drafts/saad/leadspace-test PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@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 Aug 9, 2023
@sachinmesh
Copy link

All feedback is implemented now.

@sahmad-merative @proeung @sharathmrft @Shalini-SB @keith-kaplan

@aem-code-sync
Copy link

aem-code-sync bot commented Aug 9, 2023

Page Scores Audits Google
/block-library/blocks/video PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
/drafts/saad/leadspace-test PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@proeung proeung merged commit a4fece3 into hlxsites:main Aug 9, 2023
2 checks passed
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

@helms-charity helms-charity Awaiting requested review from helms-charity

@amol-anand amol-anand Awaiting requested review from amol-anand

@nimithshetty17 nimithshetty17 Awaiting requested review from nimithshetty17

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
@sahmad-merative @proeung @sharathmrft @sachinmesh