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

Feature/video v1 #1624

Open
wants to merge 39 commits into
base: main
Choose a base branch
from
Open

Conversation

georgecamilar
Copy link

Q                       A
Fixed Issues? Basic Video Component
Patch: Bug Fix?
Minor: New Feature?
Major: Breaking Change?
Tests Added + Pass? Yes
Documentation Provided Yes (code comments and or markdown)
Any Dependency Changes?
License Apache License, Version 2.0

Video component written in HTL that renders a video reference using the standard html5 player, with features like autoplay and loop. Also a poster can be used when the video is loading.

georgecamilar and others added 16 commits June 14, 2021 11:17
* Added video readme.md file
* Adapt general README file
…nents

 * Added Video Component type to adobe data layer
 * Adapted Unit tests to match the changes
 * Added placeholder to the component
 * Added CQ Link Checker to verify the file reference
 * Unit Tests adaption
* fix getting link checker osgi service
* create reference to content page
* Adding data layer json properties export for the video component

Contributing to Adobe data layer
@georgecamilar
Copy link
Author

georgecamilar commented Jun 15, 2021

Hey, @vladbailescu! Can you please take another look at this pr? I closed my last one and open this one.
It is regarding the video component. Maybe you can guide me on what steps should i take next. Thanks a lot!

@codecov
Copy link

codecov bot commented Jun 15, 2021

Codecov Report

Merging #1624 (deb6f7e) into development (076973a) will increase coverage by 0.14%.
The diff coverage is 97.82%.

❗ Current head deb6f7e differs from pull request most recent head eb86fc4. Consider uploading reports for the commit eb86fc4 to get more accurate results
Impacted file tree graph

@@                Coverage Diff                @@
##             development    #1624      +/-   ##
=================================================
+ Coverage          86.53%   86.68%   +0.14%     
+ Complexity          2197     2163      -34     
=================================================
  Files                207      207              
  Lines               5869     5798      -71     
  Branches             879      859      -20     
=================================================
- Hits                5079     5026      -53     
+ Misses               321      312       -9     
+ Partials             469      460       -9     
Impacted Files Coverage Δ
...ts/internal/models/v1/datalayer/VideoDataImpl.java 87.50% <87.50%> (ø)
.../core/components/internal/models/v1/VideoImpl.java 100.00% <100.00%> (ø)
...com/adobe/cq/wcm/core/components/models/Video.java 100.00% <100.00%> (ø)
...cm/core/components/models/datalayer/VideoData.java 100.00% <100.00%> (ø)
.../datalayer/builder/ComponentDataLayerExtender.java 100.00% <100.00%> (ø)
...s/datalayer/builder/VideoComponentDataBuilder.java 100.00% <100.00%> (ø)
.../core/components/internal/models/v2/ImageImpl.java 82.96% <0.00%> (-0.87%) ⬇️
...nts/internal/models/v1/embeddable/YouTubeImpl.java 87.03% <0.00%> (-0.69%) ⬇️
.../core/components/internal/models/v1/ImageImpl.java 86.79% <0.00%> (-0.23%) ⬇️
...com/adobe/cq/wcm/core/components/models/Image.java 100.00% <0.00%> (ø)
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 076973a...eb86fc4. Read the comment docs.

@georgecamilar
Copy link
Author

Hey @gabrielwalt ! I created a new pr with corrections of the last code review. On the last pr of the video component, i was told by @vladbailescu that i should ask you about if this is more for contrib repo or for the core component. Also can you please tell me what the next steps are to contributing?

@vladbailescu
Copy link
Member

vladbailescu commented Jun 22, 2021

@georgecamilar , thanks again for your contribution, it's very much appreciated! As for next steps:

  1. @gabrielwalt to confirm if we should add this here or to the Contrib repo
  2. If it will be added to the Core, we'll need to do a bit more thorough review. I had a glance at the code it looks pretty much ok to me.
  3. If it will be added to Contrib, I'll arrange you get commit rights there, no need for a more formal review.

* @since com.adobe.cq.wcm.core.components.models.datalayer 1.0.0
*/
@JsonProperty("video")
default AssetData getAssetData() {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to add the poster here as well?

* **Vendor**: Adobe
* **Version**: v1
* **Compatibility**: AEM 6.4
* **Status**: work-in-progress
Copy link
Member

Choose a reason for hiding this comment

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

Please add yourself/company as contributor!

@sonarcloud
Copy link

sonarcloud bot commented Jul 28, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jkoebner
Copy link
Contributor

jkoebner commented Aug 2, 2021

@vladbailescu @gabrielwalt any updates as to when we can expect this feature to be added to the core components? would be great to know if this is going to be part of the core components for further planning of the integration of multimedia components. Kind Regards

@kwin
Copy link
Contributor

kwin commented Dec 14, 2022

@vladbailescu Regarding

thanks again for your contribution, it's very much appreciated! As for next steps:

Can you show your appreciation by either allowing this to be committed to contrib or to core components or outline what needs to be changed to somehow get this contribution integrated?

@bpauli bpauli mentioned this pull request Dec 19, 2022
@vladbailescu
Copy link
Member

@kwin , we will be taking this into one of our sprints beginning of 2023.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants