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

Added support for product videos #2433

Merged
merged 17 commits into from
Feb 21, 2019

Conversation

rain2o
Copy link
Contributor

@rain2o rain2o commented Feb 13, 2019

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
video-placeholder-before

AFTER: Video Placeholder Image
video-placeholder-after

Video Loading
video-loading

Video Playing
video-playing

Upgrade Notes and Changelog

  • No upgrade steps required (100% backward compatibility)
  • I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

Contribution and currently important rules acceptance

@rain2o
Copy link
Contributor Author

rain2o commented Feb 13, 2019

@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.

@patzick patzick added the vs-hackathon Tasks for the Hackathon label Feb 13, 2019
@patzick patzick added this to the 1.9 milestone Feb 13, 2019
Copy link
Collaborator

@patzick patzick left a 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?

core/modules/catalog/components/ProductGallery.ts Outdated Show resolved Hide resolved
core/modules/catalog/helpers/index.ts Outdated Show resolved Hide resolved
core/modules/catalog/helpers/index.ts Outdated Show resolved Hide resolved
src/themes/default/components/core/ProductGallery.vue Outdated Show resolved Hide resolved
@patzick
Copy link
Collaborator

patzick commented Feb 13, 2019

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 :)

@rain2o
Copy link
Contributor Author

rain2o commented Feb 14, 2019

Thanks for the suggestions @patzick ! I'll get this cleaned up and keep the other PR in mind while working on it.

@rain2o
Copy link
Contributor Author

rain2o commented Feb 14, 2019

@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.

@patzick
Copy link
Collaborator

patzick commented Feb 17, 2019

hey @rain2o :)
#2432 has been merged to develop, could you adjust your changes to current state?

@rain2o
Copy link
Contributor Author

rain2o commented Feb 18, 2019

Thanks for the update @patzick . I have merged with develop and pushed after cleaning up. Let me know any further thoughts on the PR

@pkarw
Copy link
Collaborator

pkarw commented Feb 18, 2019

@patzick how does the status look like in here?

Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

For me this looks good:) Great job @rain2o

But to test this on demo #2436 should be completed, because we don't have test data.

@rain2o
Copy link
Contributor Author

rain2o commented Feb 19, 2019

@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.

Copy link
Collaborator

@filrak filrak left a 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 ;)

@filrak
Copy link
Collaborator

filrak commented Feb 21, 2019

@rain2o please resolve changelog conflict and we can merge in!

@rain2o
Copy link
Contributor Author

rain2o commented Feb 21, 2019

@filrak Conflicts have been resolved. Thanks!

@filrak filrak merged commit 1a6e66c into vuestorefront:develop Feb 21, 2019
@patzick patzick modified the milestones: 1.9, 1.8.3 Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vs-hackathon Tasks for the Hackathon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants