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

[11.0][ADD] website_video_preview #439

Merged
merged 5 commits into from
Jan 23, 2019
Merged

[11.0][ADD] website_video_preview #439

merged 5 commits into from
Jan 23, 2019

Conversation

tarteo
Copy link
Member

@tarteo tarteo commented Mar 22, 2018

This module initially only shows a preview of videos instead of directly loading the iframe.
The iframe is loaded when the user clicks on the preview image.

Currently only YouTube videos are supported.

@tarteo
Copy link
Member Author

tarteo commented Sep 6, 2018

Is anyone interested in this module?

=====================

This module initially only shows a preview of videos instead of directly loading the iframe.
The iframe is loaded when the user clicks on the preview image.
Copy link

Choose a reason for hiding this comment

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

Please, improve long and short description as is not really clear what your module adds.
I had to read the code to understand :)
You should also stress a bit the advantages of not loading the IFRAME to the avg user ;)
Basically, answer the obvious question: "why should I use this module?"

var $media = $(res);

// Add preview
var $play_button = $('<img class="play_button" src="/website_video_preview/static/src/img/yt_button.png"/>');
Copy link

Choose a reason for hiding this comment

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

if the idea is to support more video provider I'd pave it by splitting this code to convenient functions.
You could have a mapping of providers and a matching regex for urls, something like:

video_providers: {
    'youtube': {'regex': $url_regex, 'handler': this.handle_youtube, 'preview_img': '/foo/baz.png'},
    'vimeo': {'regex': $url_regex, 'handler': this.handle_vimeo, 'preview_img': '/foo/bar.png'},
    [...]
}

what do you think?

@simahawk simahawk added this to the 11.0 milestone Oct 1, 2018
@simahawk
Copy link

simahawk commented Oct 1, 2018

@tarteo mind to talk about this in person? :)

@tarteo
Copy link
Member Author

tarteo commented Oct 1, 2018

@tarteo mind to talk about this in person? :)

Where are you ;)?

@tarteo
Copy link
Member Author

tarteo commented Oct 2, 2018

Hey @simahawk I refactored the code to your suggestion, what do you think?

#. Go in editing mode on the website
#. Go to the location where you want to insert a video
#. Insert the video
#. Save the page
Copy link

Choose a reason for hiding this comment

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

worth to mention how you can extend it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @simahawk I've added this to the JS files including other documentation for the functions. Can you review it? 😄

@simahawk
Copy link

simahawk commented Oct 9, 2018

@tarteo LG but I've tested it on runbot and I get the preview on video edit but no one when you close the popup

screenshot from 2018-10-09 17-41-02

image

nor when you just view the page see it in action:

http://3341687-439-90b5ea.runbot1.odoo-community.org

@tarteo
Copy link
Member Author

tarteo commented Oct 10, 2018

@simahawk The issue was that your video is really old, which has no high quality thumbnail. So what I did is replace it with 0.jpg which always works for all videos.

Copy link

@simahawk simahawk left a comment

Choose a reason for hiding this comment

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

@tarteo works now, thanks!
Overall: LG.
Just a tip, but not blocking of course: I'd use fadeIn/Out to switch between image and iframe to make the transition smoother.

@tarteo
Copy link
Member Author

tarteo commented Jan 22, 2019

@OCA/website-maintainers Can someone also review this? :)

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza pedrobaeza merged commit d288aca into OCA:11.0 Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants