hlxsites / franklin-assets-selector

Use this repository template for new Helix Pages projects.
Apache License 2.0
6 stars 5 forks source link

Video block #83

Closed yugandhar02 closed 1 month ago

yugandhar02 commented 1 month ago

Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):

Fix #

Test URLs:

aem-code-sync[bot] commented 1 month ago

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.

Commits * [0a1e30b](https://github.com/hlxsites/franklin-assets-selector/commit/0a1e30b807c2b160657a063c54fedb11370251ac) :white_check_mark: (latest) * [6141bab](https://github.com/hlxsites/franklin-assets-selector/commit/6141bab87eba1a78db3d2f06521ff5fbc48fc90c) :white_check_mark: * [c9be193](https://github.com/hlxsites/franklin-assets-selector/commit/c9be193b03a0cdee0913b760e0be39174d4c0e9f) :white_check_mark: * [d9f7f91](https://github.com/hlxsites/franklin-assets-selector/commit/d9f7f91d02712f55114a509d4e9a359f5d3156da) :white_check_mark: * [3be7e1d](https://github.com/hlxsites/franklin-assets-selector/commit/3be7e1d572eb1d865dd4858b7a619ea5f1a577e8) :white_check_mark: * [b99d334](https://github.com/hlxsites/franklin-assets-selector/commit/b99d33472874ce9a7d43b14c45518ee13e134f04) :white_check_mark: * [597ac30](https://github.com/hlxsites/franklin-assets-selector/commit/597ac30cd453ccc6a97406b30190fdbbd9041057) :white_check_mark: * [5f8c538](https://github.com/hlxsites/franklin-assets-selector/commit/5f8c538b1ef61b0edfb10bdb87213f3cd6f173f6) :white_check_mark: * [ed0f510](https://github.com/hlxsites/franklin-assets-selector/commit/ed0f5107e315e46e541dd09909366b93e5ac3020) :white_check_mark: * [0f477f4](https://github.com/hlxsites/franklin-assets-selector/commit/0f477f48fe8539ee3d19d7fa58104d1c537eb62e) :white_check_mark: * [d94ad55](https://github.com/hlxsites/franklin-assets-selector/commit/d94ad55cd784d12bc5c4d805ad726cf02b5f8105) :white_check_mark: * [543ec39](https://github.com/hlxsites/franklin-assets-selector/commit/543ec3995b4bf783f1275648bf7431a843ef06a9) :white_check_mark: * [0cdbb90](https://github.com/hlxsites/franklin-assets-selector/commit/0cdbb9042b668c64b3a81aab5b5c69bbbc816373) :white_check_mark: * [95d224b](https://github.com/hlxsites/franklin-assets-selector/commit/95d224b6071787a5390da2eaf147440db1e4f7a9) :white_check_mark: * [77a6a42](https://github.com/hlxsites/franklin-assets-selector/commit/77a6a42a64bd2fbd96cfbc188261f42196e7d5c2) :white_check_mark: * [0c3ee49](https://github.com/hlxsites/franklin-assets-selector/commit/0c3ee49de4a5a8f59efba998cf1ed2139caaeb5d) :white_check_mark: * [dbc7995](https://github.com/hlxsites/franklin-assets-selector/commit/dbc7995d73812fde16d539c218cd6917060830da) :white_check_mark: * [327a317](https://github.com/hlxsites/franklin-assets-selector/commit/327a3177071aecebd11227653bc8acf482ee5f1a) :white_check_mark: * [10d931d](https://github.com/hlxsites/franklin-assets-selector/commit/10d931db58c86113889a1bc338601d93c0b26c72) :white_check_mark: * [27814de](https://github.com/hlxsites/franklin-assets-selector/commit/27814de2fe62c2ce733829caf86f48a2e8955e83) :white_check_mark: * [d639fa2](https://github.com/hlxsites/franklin-assets-selector/commit/d639fa2b305f0579edaff0a32a2ec94859a06db4) :white_check_mark: * [672c1a5](https://github.com/hlxsites/franklin-assets-selector/commit/672c1a5f659f4feec9c8fbbf3c281d58e11f2e0d) :white_check_mark: * [4afb3d2](https://github.com/hlxsites/franklin-assets-selector/commit/4afb3d2b680227f0de1d0d98ff74b906ffcb6af7) :white_check_mark: * [a348609](https://github.com/hlxsites/franklin-assets-selector/commit/a348609480558b9d88df7ff9cc3debc989624e67) :white_check_mark:
shrotia commented 1 month ago

@yugandhar02 can you please share a test URL to see the Video player in action?

shrotia commented 1 month ago

Also, imho we should have a block-level config to render Video as a Modal (default) or Inline (Autoplay On / Off). As customer has been raising performance concerns when Videos are rendered Inline.

IMHO Default behaviour: videos should not start (No auto play) & only poster image is displayed. O click on the Poster Image, and the Video Player should get launched as a Modal. In case Poster image is not available for the asset, the default fallback image should appear.

Then there should be config (at block level) to allow Auto Play or Inline video player embed.

aem-code-sync[bot] commented 1 month ago
Page Scores Audits Google
/volvo-videos/ PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI
sdmcraft commented 1 month ago

The PR seems to be failing PSI because of LCP hit due to video as hero. I think having a poster would help.

shrotia commented 1 month ago

Overall it looks good, I do not see media queries here. Please ensure the look & feel of the Tablet/Mobile are also intact.