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

Saas 12.2 elearning fixes qmo #31351

Open
wants to merge 3 commits into
base: saas-12.2
from

Conversation

Projects
None yet
5 participants
@qmo-odoo
Copy link

qmo-odoo commented Feb 22, 2019

Description of the issue/feature this PR addresses: Various fixes, cleaning and improvements for elearning module

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

@robodoo robodoo added the seen 🙂 label Feb 22, 2019

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-fixes-qmo branch Feb 22, 2019

@dbeguin

This comment has been minimized.

Copy link
Contributor

dbeguin commented Feb 22, 2019

To avoid having to much css class that make the xml template unclear, you can just give the class to the top parent and use css navigation and less power to apply style to the children elements. That way you also reduce the scss file size.

XML :

<div class="o_wslides_slide_header">
    <div>
        <div>

scss :

.o_wslides_slide_header {
    background-color: #62495B;
    height: 200px;
    > div {
        width: 1000px;
        margin: 0 auto;
        height: 100%;
        > div {
            display: flex;
            height: 100%;
            width: 50%;
            flex-direction: column;
            justify-content: flex-end;
            padding-bottom: 50px;
        
            a {
                color: #fff;
                font-size: 2.5rem;
                text-decoration: none  !important;
            }
        }
    }
}

I think you can apply this rule for the rest of classes you added in all the templates.
Just watch out, keep the class used for js selector

@C3POdoo C3POdoo added the RD label Feb 22, 2019

@dbeguin
Copy link
Contributor

dbeguin left a comment

\o/ cleaner, fewer code! weeeeeeeeeeeeee

color: $primary !important;
cursor: pointer;
}
}

This comment has been minimized.

@dbeguin

dbeguin Feb 22, 2019

Contributor

lint : last line is missing

addons/website_slides/static/src/scss/slides_slide_fullscreen.scss Outdated
@@ -118,7 +118,7 @@

This comment has been minimized.

@dbeguin

dbeguin Feb 22, 2019

Contributor

you can remove all the blank lines inside a class definition

addons/website_slides/controllers/main.py Outdated
slide_partner.sudo().write({
'quiz_attempts_count': slide_partner.quiz_attempts_count if not bad_answers else slide_partner.quiz_attempts_count + 1,
'points_won': points if not bad_answers else 0,
'completed': not bad_answers
})
user = {}

This comment has been minimized.

@dbeguin

dbeguin Feb 22, 2019

Contributor

I would name it user_karma or karma_progress_bar as it's information about user karma for the progression bar and not about the user itself.

addons/website_slides/static/src/js/slides_course_quiz.js Outdated
@@ -156,14 +156,14 @@ odoo.define('website_slides.quiz', function (require) {
self._updateProgressbar();
$('#check-'+self.slide.id).replaceWith($('<i class="check-done o_wslides_slide_completed fa fa-check-circle"></i>'));
self.slide.done = true;
self._renderSuccessModal();
self._renderSuccessModal(data);

This comment has been minimized.

@dbeguin

dbeguin Feb 22, 2019

Contributor

I would only give to the modal the data really used in the template : points and user_karma.

addons/website_slides/static/src/js/slides_course_quiz.js Outdated
@@ -105,10 +106,10 @@ odoo.define('website_slides.quiz', function (require) {
var questionID =$('#answer'+ answers.goodAnswers[i] +' .o_wslides_quiz_radio_box input').attr('question_id');

This comment has been minimized.

@dbeguin

dbeguin Feb 22, 2019

Contributor

while you're at it to add some spaces -> var questionID = $... :)

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 22, 2019

@jem-odoo
Copy link
Contributor

jem-odoo left a comment

Small comment for you to start your day ;)
Looks like we got a good start. On lache rien ! :D

//--------------------------------------------------------------------------
// Handler
//--------------------------------------------------------------------------
_onClickToggleSidebar: function (ev) {

This comment has been minimized.

@jem-odoo

jem-odoo Feb 26, 2019

Contributor

You got indent problem here ;)

This comment has been minimized.

@qmo-odoo
addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
// Public
//--------------------------------------------------------------------------
updateTitle: function (data) {
$('.o_wslides_fs_slide_title').empty().html(QWeb.render('website.course.fullscreen.title', data));

This comment has been minimized.

@jem-odoo

jem-odoo Feb 26, 2019

Contributor

This time, you should be able to use this.$()

This comment has been minimized.

@qmo-odoo
addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
_onClickTab: function (ev) {
// trigger_up the event with slide id
var slideIndex = parseInt($(ev.currentTarget).attr('index'));
this.trigger_up('change_slide', {slideIndex: slideIndex});

This comment has been minimized.

@jem-odoo

jem-odoo Feb 26, 2019

Contributor

I think it will be simpler to use slide_id rather than index.

This comment has been minimized.

@qmo-odoo
addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
_fetchSlideContent: function (){
var def = $.Deferred();
if ((this.slide.slide_type === 'quiz' || this.slide.has_quiz) && !this.slide.quiz ){
def = this._fetchQuiz();

This comment has been minimized.

@jem-odoo

jem-odoo Feb 26, 2019

Contributor

This will be handle directly by quiz Widget

addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
if (self.slide.slide_type === "video"){
embed_url = "https://" + embed_url + "&rel=0&autoplay=1&enablejsapi=1&origin=" + window.location.origin;
}
$('.o_wslides_fs_player').html(QWeb.render('website.slides.fullscreen', {

This comment has been minimized.

@jem-odoo

jem-odoo Feb 26, 2019

Contributor

When rendering sub part of widget, try to only give {'widget': this} as render value. So in the template you can access widget.whatever_you_want ;)

addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
init: function (el, slides, currentSlideIndex){
this.slides = slides;
this.index = currentSlideIndex;
this.slide = this.slides[this.index];

This comment has been minimized.

@jem-odoo

jem-odoo Feb 26, 2019

Contributor

You should have this.set('slide', false); this.on('change:slide, this._onchangeSlide). The handler _onchangeSlide will be called when slide is changed, so in there, you can do something like

this.renderContent().then(function(){
  self.header.setTitle();
  if (need to set as done according to type) {
          self.setAsDone();
   }
});

You should have a method to find the slide in the slide list from its slide_id. Look at https://underscorejs.org/#findIndex, maybe that can help you. (this lib exists in odoo, so use it, this is very usefull)

This comment has been minimized.

@qmo-odoo
addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
var self = this;
this.header = new Header(this);
this.sidebar = new Sidebar(this);

This comment has been minimized.

@jem-odoo

jem-odoo Feb 26, 2019

Contributor

Don't forget to give the param slideCompletionMap

var slideCompletionMap = {};
_.each(slides, function(slide, index) {
   slideCompletionMap[slide.id] = slide.completion;
});
addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
* Creates slides objects from every slide-list-cells attributes
*/
_getSlides: function (){
var slides = $('.o_wslides_fs_sidebar_slide_tab');

This comment has been minimized.

@jem-odoo

jem-odoo Feb 26, 2019

Contributor

you should be able to use this.$() here too

addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
var slideList = [];
for (var i = 0; i < slides.length;i++){
var slide = $(slides[i]);
slideList.push(slide.data());

This comment has been minimized.

@jem-odoo

jem-odoo Feb 26, 2019

Contributor

As data here, we should have name, type, hasQuiz, completed, id, ... and some specific (embed url, ...) for specific type.

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 26, 2019

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-fixes-qmo branch Feb 28, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Feb 28, 2019

@jem-odoo
Copy link
Contributor

jem-odoo left a comment

Remarks and questions.

addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
*/
_onClickMiniQuiz: function (ev){
var slideID = parseInt($(ev.currentTarget).attr('slide_id'));
this.trigger_up('click_miniquiz', {slideID: slideID});

This comment has been minimized.

@jem-odoo

jem-odoo Mar 1, 2019

Contributor

I would trigger the same event change_slide but with an additionnal flag "quizMode" set to true.

This comment has been minimized.

@qmo-odoo
addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
* @param {*} ev
*/
_onClickMiniQuiz: function (ev){
var slideID = parseInt($(ev.currentTarget).attr('slide_id'));

This comment has been minimized.

@jem-odoo

jem-odoo Mar 1, 2019

Contributor

use data api instead ;)

This comment has been minimized.

@qmo-odoo
addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
this.url = undefined;
this.urlToSmallScreen = undefined;
this.activetab = undefined;
this.player = undefined;
this.goToQuiz = false;

This comment has been minimized.

@jem-odoo

jem-odoo Mar 1, 2019

Contributor

Should be a key in slide dict, so when changing it, you will re-render the slide content and display the related quiz

addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
this._bindListEvents();
$(document).keydown(this._onKeyDown.bind(this));
return this._super.apply(this, arguments);
return $.when(def.then(function (){

This comment has been minimized.

@jem-odoo

jem-odoo Mar 1, 2019

Contributor

no need when() here

addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
if ((self.slide.slide_type === "presentation" || self.slide.slide_type === "document" || self.slide.slide_type === "infographic" || self.slide.slide_type === "webpage") && !self.slide.quiz){
var currentSlide = self.get('slide');
self._setPreviousAndNextSlides();
self.quizOnSlide = currentSlide.has_quiz && currentSlide.slide_type !== "quiz";

This comment has been minimized.

@jem-odoo

jem-odoo Mar 1, 2019

Contributor

seems not used

This comment has been minimized.

@qmo-odoo
var completion = _.min([this.channelCompletion, 100]);
this.$('.o_wslides_fs_sidebar_progressbar .progress-bar').css('width', completion + "%" );
this.$('.o_wslides_progress_percentage').text(completion);
$(self.get('slide').htmlContent).appendTo('.o_wslides_fs_webpage_content');

This comment has been minimized.

@jem-odoo

jem-odoo Mar 1, 2019

Contributor

there should be on div to handle the content, and each slide type should empty that div and put there content inside.

addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
this._updateUrl();
history.pushState(null,'',this.url);
this.quizOnSlide = currentSlide.has_quiz && currentSlide.slide_type !== "quiz";
return $.when($, self._fetchSlideContent()).then(function (){

This comment has been minimized.

@jem-odoo

jem-odoo Mar 1, 2019

Contributor

no need when() here and after. IMHO

addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
this.slides = slides;
this.set('slide', this._findSlideByID(currentSlideID));
this.on('change:slide', this, this._onChangeSlide);
this.quizOnSlide = false;
this.url = undefined;
this.urlToSmallScreen = undefined;

This comment has been minimized.

@jem-odoo

jem-odoo Mar 1, 2019

Contributor

what is the purpose of this attribute ?

addons/website_slides/static/src/js/slides_course_fullscreen_player.js Outdated
this.embed_url = "https://" + this.embed_url + "&rel=0&autoplay=1&enablejsapi=1&origin=" + window.location.origin;
}
var defs = [];
defs.push($('.o_wslides_fs_player').html(QWeb.render('website.slides.fullscreen', {

This comment has been minimized.

@jem-odoo

jem-odoo Mar 1, 2019

Contributor

we should use template rendering only for some slide type (not quiz, not certif, and not video).

}
var Quiz = new QuizWidget(this, self.get('slide'), self.nextSlide);
Quiz.appendTo('.o_wslides_fs_player');
this.$('.next-slide').click(function (){

This comment has been minimized.

@jem-odoo

jem-odoo Mar 1, 2019

Contributor

should be handled by trigger_up of quiz widget

qmo-odoo added some commits Feb 21, 2019

[IMP] website_slides: Renamed css classes + deleted sidebar widget
As the sidebar toggling mechanic is only present in fullscreen mode,
it will be handled inside the the fullscreen widget itself
[REF] website_slides: Refactoring of the fullscreen widget
Separated the header and the sidebar into their own separate widget
and let them handle their dedicated functionalities

Cleaned and reorganised the file to be more guidelines compliant

Added deferred to avoid some undesirable async issues

@qmo-odoo qmo-odoo force-pushed the odoo-dev:saas-12.2-elearning-fixes-qmo branch to d67a776 Mar 5, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Mar 5, 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.