-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Added support for product videos #2433
Conversation
@patzick Sorry for the delay on this! Had some network issues on the ride home from the hackathon. But I finally resolved the URL issue we had so I believe this is ready now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rain2o, great that you've finished this feature! From screens looks really great :)
Also i'm glad that you safely get back to home!
I've made a CR and have some suggestions. The most important question - what do you think about moving videoData recognition to data importing module as well (or to API)? Just to serve to frontend exactly what it needs?
oh, bad thing i see now that we'll have conflict with #2432 because it changes gallery component. That's why video player have to be a separate component, then this wouldn't collide that much. Please keep in mind that PR too to avoid additional work on this feature :) |
Thanks for the suggestions @patzick ! I'll get this cleaned up and keep the other PR in mind while working on it. |
@patzick I have separated the video logs into a separate component, but after looking at that other PR it looks like he created a new component where the carousel is created. In order to make mine work with his I'll need to merge his updates. I guess the best approach is wait for that PR to be merged into develop then I can update my branch with develop. |
Thanks for the update @patzick . I have merged with develop and pushed after cleaning up. Let me know any further thoughts on the PR |
@patzick how does the status look like in here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaanKouters just pointed out that I forgot to add the video component to the zoom carousel. I'll get that added in before we move forward with this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overally great PR, thank you! I had some troubles understanding hideImage
contextual purpose at the beginning therefore I think it's good to name it in a more descriptive way but it's just a small thing ;)
src/themes/default/components/core/ProductGalleryZoomCarousel.vue
Outdated
Show resolved
Hide resolved
@rain2o please resolve changelog conflict and we can merge in! |
@filrak Conflicts have been resolved. Thanks! |
Related issues
#2388 and mage2vuestorefront#71
Short description and why it's useful
This PR adds the ability to load and play videos in the product gallery from YouTube and Vimeo.
Note - As a part of this I created a new component in the default theme called
LoaderScoped
. It can be used to provide the same style loading indicator inside of a scoped area without triggering the full page loader. I thought this could be useful in other areas potentially and needed to indicate the video was being loaded without taking the whole page out.Screenshots of visual changes before/after (if there are any)
BEFORE: Video Placeholder Image
AFTER: Video Placeholder Image
Video Loading
Video Playing
Upgrade Notes and Changelog
Contribution and currently important rules acceptance