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

[IMP] website: Add a video as a background #30014

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@var-odoo
Copy link

var-odoo commented Jan 8, 2019

Description of the issue/feature this PR addresses:

Current behavior before PR:

  • User doesn't get the background video as a option like we already have a background image/color.
  • Media block is used for background video.

Desired behavior after PR is merged:

  • Now user will get Background Video" option in standard website and Media block is removed from the snippet.
  • Radio button "muted" will be added and all other radio buttons are already check by default
  • In standard website "loading background" with dropdown selection, "opacity" with scroll and radio buttons for fill/put video in container will be available same as Media block.

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@robodoo robodoo added the CI 🤖 label Jan 8, 2019

@C3POdoo C3POdoo added the RD label Jan 8, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch Jan 23, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 23, 2019

@robodoo robodoo removed the CI 🤖 label Feb 20, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch 2 times, most recently to 236889a Feb 20, 2019

@robodoo robodoo added the CI 🤖 label Feb 21, 2019

@robodoo robodoo removed the CI 🤖 label Mar 5, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch to d09bee2 Mar 5, 2019

@robodoo robodoo added the CI 🤖 label Mar 5, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch from d09bee2 to 2acc6a3 Mar 18, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 18, 2019

@robodoo robodoo removed the CI 🤖 label Mar 26, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch from 49c5bef to cbded11 Mar 26, 2019

@robodoo robodoo added the CI 🤖 label Mar 26, 2019

@@ -519,4 +519,4 @@ img.o_we_custom_image.mx-auto {
margin: 0;
padding: 0;
border: 0;
}

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 5, 2019

Contributor

There should be a new line at the end of the file ;)

this.$('textarea#o_video_text').val(src);

this.$('input#o_video_autoplay').prop('checked', src.indexOf('autoplay=1') >= 0);
this.$('input#o_video_hide_controls').prop('checked', src.indexOf('controls=0') >= 0);
this.$('input#o_video_loop').prop('checked', src.indexOf('loop=1') >= 0);
this.$('input#o_video_hide_fullscreen').prop('checked', src.indexOf('fs=0') >= 0);
this.$('input#o_video_hide_yt_logo').prop('checked', src.indexOf('modestbranding=1') >= 0);
this.$('input#o_video_muted').prop('checked', src.indexOf('mute=1') >=0);

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 5, 2019

Contributor

Missing space around operators, check your linter, it should have told you that mistake 😉
https://github.com/odoo/odoo/wiki/Javascript-coding-guidelines

@@ -798,6 +798,7 @@ registry.colorpicker = SnippetOption.extend({
/**
* Handles the edition of snippet's background image.
*/

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 5, 2019

Contributor

No need of that extra line

</div>
<div class="o_dm_option">
<label class="o_switch mb0"><input id="o_video_hide_dm_logo" type="checkbox"/><span/>Hide Dailymotion logo</label>
</div>
<div class="o_dm_option">
<label class="o_switch mb0"><input id="o_video_hide_dm_share" type="checkbox"/><span/>Hide sharing button</label>
</div>
<div class="row o_yt_option o_dm_option o_vim_option">
<label class="col-md-4 pre_label">Video background</label>
<label class="col-md-7">

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 5, 2019

Contributor

Should not it be col 4 + 8 or 5 + 7 ?

<div class="o_yt_option o_dm_option o_vim_option custom-control custom-radio col-md-6">
<input type="radio" id="fitIframe" name="iframefit" class="custom-control-input" value="fitIframe"/>
<label for="fitIframe" class="custom-control-label">
<small>Put video in container</small>

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 5, 2019

Contributor

I would not use small to avoid breaking the alignements. But if the alignments are not broken, use small as a label class, not an inside DOM element.

check: async function () {
await testUtils.dom.triggerEvents($('.note-image-popover .note-float .note-icon-align-center'), ['mousedown', 'click']);
await testUtils.dom.triggerEvents($('.note-image-popover .note-float .note-icon-align-left'), ['mousedown', 'click']);
await testUtils.dom.triggerEvents($('.note-image-popover .note-float .note-icon-align-left'), ['mousedown', 'click']);

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 5, 2019

Contributor

Extra spaces

Show resolved Hide resolved addons/website/static/src/js/content/snippets.animation.js
@@ -498,6 +498,235 @@ registry.slider = publicWidget.Widget.extend({
this._computeHeights();
},
});
registry.backgroundVideo = Animation.extend({
selector : ".oe_video_background",
disabledInEditableMode : false,

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 5, 2019

Contributor

Extra space -> linter

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 11, 2019

Contributor

:)

}

return new Promise(function (resolve, reject) {
$el.on('YTPReady', function () {

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 5, 2019

Contributor

--> Linter

@robodoo robodoo removed the CI 🤖 label Apr 5, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch 2 times, most recently from 08867f1 to d523a14 Apr 10, 2019

this.$('input#o_video_hide_dm_logo').prop('checked', src.indexOf('ui-logo=0') >= 0);
this.$('input#o_video_hide_dm_share').prop('checked', src.indexOf('sharing-enable=0') >= 0);

if(src){

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 11, 2019

Contributor

https://github.com/odoo/odoo/wiki/Javascript-coding-guidelines#use-a-linter :)
Is this case possible ? There should always be a src on a media being edited ?

firstFilters: ['background'],
res_model: $editable.data('oe-model'),
res_id: $editable.data('oe-id'),
o_yt_option: true,
o_dm_option: true,
o_vim_option: true,

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 11, 2019

Contributor

I don't understand what this is. There should only be a new option to say "I want options for background videos"

this.$target.trigger('content_changed');
_editor.on('save', this, function (data) {
if (_editor.$el.find('.tab-pane.active').attr('id') === 'editor-media-image') {
if (this.$target.find(".yt_video_container").length) {

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 11, 2019

Contributor

$target.find( -> $(

this._setCustomBackground($image.attr('src'));
this.$target.trigger('content_changed');
_editor.on('save', this, function (data) {
if (_editor.$el.find('.tab-pane.active').attr('id') === 'editor-media-image') {

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 11, 2019

Contributor

$el.find( -> $(

But in fact I do not understand the following code, you should not have to check the DOM of the modal being saved to know which tab is active. The modal is saved and closed and give you some image / video... and should only have to work with it.

this.$target.find(".yt_video_container").remove();
this.$target.removeAttr('src opacity background iframefit muted loop');
this.$target.removeClass('oe_video_background oe_video');
$(this.$target.children().first()).removeClass("oe_video_bg");

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 11, 2019

Contributor

Wrapping a jquery element in a jquery element ?

});
_editor.on('closed', this, function () {
$image.remove();
if (_editor.$el.find('.tab-pane.active').attr('id') === 'editor-media-image') {

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 11, 2019

Contributor

Why? Is not the image always to be removed?

@@ -498,6 +498,235 @@ registry.slider = publicWidget.Widget.extend({
this._computeHeights();
},
});
registry.backgroundVideo = Animation.extend({
selector : ".oe_video_background",
disabledInEditableMode : false,

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 11, 2019

Contributor

:)

@-webkit-keyframes o-anim_movingBG { @include o-anim-movingBG; }
@-moz-keyframes o-anim_movingBG { @include o-anim-movingBG; }
@-ms-keyframes o-anim_movingBG { @include o-anim-movingBG; }
@-o-keyframes o-anim_movingBG { @include o-anim-movingBG; }

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 11, 2019

Contributor

No need of vendor prefixes for keyframes, we do not support the browsers that do not support keyframes.

@@ -77,6 +77,7 @@

<!-- Custom empty file for user javascript -->
<script type="text/javascript" src="/website/static/src/js/user_custom_javascript.js"/>
<script type="text/javascript" src="/website/static/lib/YTPlayer/jquery.mb.YTPlayer.js"/>

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 11, 2019

Contributor

Lazy load the library only when needed (see jsLibs properties of widgets).

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch from d523a14 to 8f861d2 Apr 15, 2019

@robodoo robodoo added the CI 🤖 label Apr 15, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch from 8f861d2 to 41ce5fd Apr 15, 2019

@robodoo robodoo removed the CI 🤖 label Apr 15, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch from 41ce5fd to 1c1ed01 Apr 15, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Apr 15, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch from c852ce9 to 9ba8598 Apr 16, 2019

@robodoo robodoo added the CI 🤖 label Apr 16, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch from 9ba8598 to f7bc919 Apr 16, 2019

@robodoo robodoo removed the CI 🤖 label Apr 16, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch 4 times, most recently from 44d252e to 90be8b5 Apr 16, 2019

@robodoo robodoo added the CI 🤖 label Apr 18, 2019

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch from 90be8b5 to d84bc61 Apr 19, 2019

@robodoo robodoo removed the CI 🤖 label Apr 19, 2019

var-odoo added some commits Jan 8, 2019

[IMP] website: add a video as a background
added
- muted option and check all other options by default
- loading background with dropdown selection
- opacity with scroll
- radio buttons to fill/put video in container
[IMP]website: updated youtube library
- youtube library is updated.
- $.Deferred is not used anymore so it is replaced with promise.
[IMP]website, web_editor: improvement in video options
- loading background, opacity and filling options are also available for other video types like dailymotion, vimeo not only for the youtube video.
[IMP]website, web_editor: improvement in code
- background video option for adding video as background is removed.
- background image option is renamed as "background" and media dialog will be open with both images and video tabs.
- loading background, opacity and filling options will not be displayed while adding a normal video in website.
[IMP] web_editor: Save previous URL of video
- If user wants to change the video, the previous URL and options are saved

@robodoo robodoo added the CI 🤖 label Apr 19, 2019

@qsm-odoo
Copy link
Contributor

qsm-odoo left a comment

Here are some comments and questions. Also, can you tell me if using the yt library is necessary or if we would have the same results if we do not use it ? Thank you.

this._removeVideo();
this._setCustomBackground($image.attr('src'));
} else {
if ($('textarea#o_video_text').val() === "") {

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 19, 2019

Contributor

This condition should check something on data, not check the whole DOM again

}

if (data) {
var src = $(data).attr('data-oe-expression');

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 19, 2019

Contributor

Why data-oe-expression ?

* @private
*/
_removeVideo: function () {
this.$(".yt_video_container").remove();

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 19, 2019

Contributor

Use single quotes for all technical strings (') (only the ones you add yourself)


//--------------------------------------------------------------------------
// Public
//--------------------------------------------------------------------------

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 19, 2019

Contributor

The lifecycle of the widget init, willStart, start, ... should not be in a Public section (no section at all). You have no public comment to add here

*/
destroy: function () {
this._super.apply(this, arguments);
this._stopVideo();

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 19, 2019

Contributor

Stop the video before calling _super, probably the same in your case but it will be safer if we change something in the future.

var videoUrl;

// mobile phones are not supported, just show the background image.
if (jQuery.browser.mobile) { return; }

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 19, 2019

Contributor

Can't we remove this now? This was a goal of the task to make it work in mobile :)

*
* @private
*/
_updateVideoType: function () {

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 19, 2019

Contributor

This method seems to be a duplicate of what is done in the editor to parse the video URL... can't we share something in utility functions ?

/**
* @private
*/
_createYoutubeVideo: function () {

This comment has been minimized.

Copy link
@qsm-odoo

qsm-odoo Apr 19, 2019

Contributor

This is not a handler

[IMP] website, web_editor: Improvement in code
- While selecting background colour, background video will be reset.
- Removed loading background, fill/put video in container options.

@var-odoo var-odoo force-pushed the odoo-dev:master-website-background-video-var branch from d84bc61 to fc39a23 Apr 19, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.