Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-32657

Section dragdrop: make possible to define elements swapping in the course format

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      One way to test if it works, is to install one of non-core formats and make sure that neither section nor resource drag-drop works for it.
      The simpler way, is to comment out:

          // Include course format js module
          //$PAGE->requires->js('/course/format/topics/format.js');

      at the bottom of course/format/topics/format.php and ensure that drag-drop stopped working for topics format, but still works for weeks.

      Show
      One way to test if it works, is to install one of non-core formats and make sure that neither section nor resource drag-drop works for it. The simpler way, is to comment out: // Include course format js module //$PAGE->requires->js('/course/format/topics/format.js'); at the bottom of course/format/topics/format.php and ensure that drag-drop stopped working for topics format, but still works for weeks.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32657-master-1

      Description

      As discussed in MDL-31052, it should be possible to define on the format level which elements should be modified as a result of nodes swapping. The idea is to have a yui module or js file per format, which will have the required functions.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            kabalin Ruslan Kabalin added a comment -

            The patch is attached. It allows to add format.js (see example for weeks format) with functions:

            M.course.format.get_section_selector - returns the section selector (li.section is used by default, see dragdrop.js in course/yui directory)

            M.course.format.swap_sections - performs the section swap when it is dragged to the new location, this function takes section numbers (not IDs) as parameters and performs section swapping. Again, the default actions (as they were originally) are defined in dragdrop.js in the course/yui.

            Show
            kabalin Ruslan Kabalin added a comment - The patch is attached. It allows to add format.js (see example for weeks format) with functions: M.course.format.get_section_selector - returns the section selector (li.section is used by default, see dragdrop.js in course/yui directory) M.course.format.swap_sections - performs the section swap when it is dragged to the new location, this function takes section numbers (not IDs) as parameters and performs section swapping. Again, the default actions (as they were originally) are defined in dragdrop.js in the course/yui.
            Hide
            gb2048 Gareth J Barnard added a comment - - edited

            Dear Ruslan,

            Thank you .

            I cannot see in weeks/format.php where the CSS selector 'course-content' is in relation to its definition in 'M.course.format.swap_sections'? Does this function have to move the component parts of the section as well as the encompassing section as defined by:

            M.course.format.get_section_selector = function(Y) {
                return 'li.section';
            }

            ?

            As I would have:

            M.course.format.get_section_selector = function(Y) {
                return 'tr.section';
            }

            and then need to do (plus would this work?):

            M.course.format.swap_sections = function(Y, node1, node2) {
                var CSS = {
                    COURSECONTENT : 'course-content',
                    LEFT : 'left',
                    RIGHT : 'right',
                };
             
                var sectionlist = Y.Node.all('.'+CSS.COURSECONTENT+' '+M.course.format.get_section_selector(Y));
                // Swap left block
                sectionlist.item(node1).one('.'+CSS.LEFT).swap(sectionlist.item(node2).one('.'+CSS.LEFT));
                // Swap right block
                sectionlist.item(node1).one('.'+CSS.RIGHT).swap(sectionlist.item(node2).one('.'+CSS.RIGHT));
             
                var togglelist = Y.Node.all('.'+CSS.COURSECONTENT+' '+ 'tr.cps');  // Or can use M.course.format.get_section_selector(Y) with '.previousSibling' below???
                // Swap left block
                togglelist.item(node1).one('.'+CSS.LEFT).swap(togglelist.item(node2).one('.'+CSS.LEFT));
                // Swap right block
                togglelist.item(node1).one('.'+CSS.RIGHT).swap(togglelist.item(node2).one('.'+CSS.RIGHT));
            }

            Show
            gb2048 Gareth J Barnard added a comment - - edited Dear Ruslan, Thank you . I cannot see in weeks/format.php where the CSS selector 'course-content' is in relation to its definition in 'M.course.format.swap_sections'? Does this function have to move the component parts of the section as well as the encompassing section as defined by: M.course.format.get_section_selector = function(Y) { return 'li.section'; } ? As I would have: M.course.format.get_section_selector = function(Y) { return 'tr.section'; } and then need to do (plus would this work?): M.course.format.swap_sections = function(Y, node1, node2) { var CSS = { COURSECONTENT : 'course-content', LEFT : 'left', RIGHT : 'right', };   var sectionlist = Y.Node.all('.'+CSS.COURSECONTENT+' '+M.course.format.get_section_selector(Y)); // Swap left block sectionlist.item(node1).one('.'+CSS.LEFT).swap(sectionlist.item(node2).one('.'+CSS.LEFT)); // Swap right block sectionlist.item(node1).one('.'+CSS.RIGHT).swap(sectionlist.item(node2).one('.'+CSS.RIGHT));   var togglelist = Y.Node.all('.'+CSS.COURSECONTENT+' '+ 'tr.cps'); // Or can use M.course.format.get_section_selector(Y) with '.previousSibling' below??? // Swap left block togglelist.item(node1).one('.'+CSS.LEFT).swap(togglelist.item(node2).one('.'+CSS.LEFT)); // Swap right block togglelist.item(node1).one('.'+CSS.RIGHT).swap(togglelist.item(node2).one('.'+CSS.RIGHT)); }
            Hide
            gb2048 Gareth J Barnard added a comment -

            I'm a muppet, just found 'course-content' css class. However my other questions still stand

            Show
            gb2048 Gareth J Barnard added a comment - I'm a muppet, just found 'course-content' css class. However my other questions still stand
            Hide
            gb2048 Gareth J Barnard added a comment -

            In the old 'section_classes.js' the following code:

                    if (YAHOO.util.Dom.hasClass(this.getEl(),'current')) {
                        highlightbutton = main.mk_button('div', main.portal.icons['marked'], main.getString('marked', this.sectionId));
                    } else {
                        highlightbutton = main.mk_button('div', main.portal.icons['marker'], main.getString('marker', this.sectionId));
                    }

            in the function 'section_class.prototype.init_buttons' made the 'highlightbutton' refer to 'topics' in its wording. Please can the new code refer to 'section' so it is generic across different formats .

            Thanks,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - In the old 'section_classes.js' the following code: if (YAHOO.util.Dom.hasClass(this.getEl(),'current')) { highlightbutton = main.mk_button('div', main.portal.icons['marked'], main.getString('marked', this.sectionId)); } else { highlightbutton = main.mk_button('div', main.portal.icons['marker'], main.getString('marker', this.sectionId)); } in the function 'section_class.prototype.init_buttons' made the 'highlightbutton' refer to 'topics' in its wording. Please can the new code refer to 'section' so it is generic across different formats . Thanks, Gareth
            Hide
            poltawski Dan Poltawski added a comment -

            Makes sense, only question is whether expecting exceptions like this is the right approach to doing something like that.

            Show
            poltawski Dan Poltawski added a comment - Makes sense, only question is whether expecting exceptions like this is the right approach to doing something like that.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Dear Dan,

            To answer your question, I consider this not to be an exception but to be in the spirit of Moodle in terms of being Modula to support lots of different things and to facilitate the user choice in picking / choosing what they want, Object Orientated to facilitate the integration of additional modules, Dynamic for integration with minimal effort and no core changes for their Learning Environment - which is after all what 'Moodle' stands for .

            I need to be able to override certain functions so that I can still support AJAX in my format and thus maintain the user experience. I think that Moodle would want to encourage and support diversity in order to maintain its popularity. Providing third-party developers with a credible SDK gives them oxygen upon which to breathe and embrace.

            From my point of view I realise that I do not use 'li.section' and indeed use the frowned 'table' for structure. But I am in effect dealing with a tabular layout of rows and columns - what the specification of the 'table' tag states that it is for. It also works the best when dealing with a variety of web browsers for the given functionality.

            I am happy to change and adapt my code to developments as long as I am able to provide the same basic functionality as the standard formats .

            Therefore please let this go ahead and support its requirement .

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Dear Dan, To answer your question, I consider this not to be an exception but to be in the spirit of Moodle in terms of being Modula to support lots of different things and to facilitate the user choice in picking / choosing what they want, Object Orientated to facilitate the integration of additional modules, Dynamic for integration with minimal effort and no core changes for their Learning Environment - which is after all what 'Moodle' stands for . I need to be able to override certain functions so that I can still support AJAX in my format and thus maintain the user experience. I think that Moodle would want to encourage and support diversity in order to maintain its popularity. Providing third-party developers with a credible SDK gives them oxygen upon which to breathe and embrace. From my point of view I realise that I do not use 'li.section' and indeed use the frowned 'table' for structure. But I am in effect dealing with a tabular layout of rows and columns - what the specification of the 'table' tag states that it is for. It also works the best when dealing with a variety of web browsers for the given functionality. I am happy to change and adapt my code to developments as long as I am able to provide the same basic functionality as the standard formats . Therefore please let this go ahead and support its requirement . Cheers, Gareth
            Hide
            poltawski Dan Poltawski added a comment -

            Hi Gareth,

            Sorry I was not clear, I was referring to exceptions the programming language construct:

            try {
               var sectionlistselector = '.'+CSS.COURSECONTENT+' '+M.course.format.get_section_selector(Y);
            } catch (e) {
               var sectionlistselector = '.'+CSS.COURSECONTENT+' li.'+CSS.SECTION;
            }

            Note that I agree with you about plugability, although I also note that the existing javascript was not really a public API.

            Show
            poltawski Dan Poltawski added a comment - Hi Gareth, Sorry I was not clear, I was referring to exceptions the programming language construct: try { var sectionlistselector = '.'+CSS.COURSECONTENT+' '+M.course.format.get_section_selector(Y); } catch (e) { var sectionlistselector = '.'+CSS.COURSECONTENT+' li.'+CSS.SECTION; } Note that I agree with you about plugability, although I also note that the existing javascript was not really a public API.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            I'm inclined to agree, we should be able to do it with this, though it's a little messier:

            if (M.course.format && M.course.form.get_section_selector && typeof(M.course.get_section_selector) == 'function') {
               var sectionlistselector = '.' + CSS.COURSECONTENT + ' ' + M.course.format.get_section_selector(Y);
            } else {
               var sectionlistselector = '.' + CSS.COURSECONTENT + ' li.' + CSS.SECTION;
            }

            Show
            dobedobedoh Andrew Nicols added a comment - I'm inclined to agree, we should be able to do it with this, though it's a little messier: if (M.course.format && M.course.form.get_section_selector && typeof(M.course.get_section_selector) == 'function') { var sectionlistselector = '.' + CSS.COURSECONTENT + ' ' + M.course.format.get_section_selector(Y); } else { var sectionlistselector = '.' + CSS.COURSECONTENT + ' li.' + CSS.SECTION; }
            Hide
            gb2048 Gareth J Barnard added a comment - - edited

            Dan - no worries

            Dan and Andrew, TBH, I am now a little confused on the solution! I have two 'tr' tag containers containing the left, content and right css class 'td' tags. I need to swap them both at the same time. Each pair represents a single section. Their respective classes are 'cps' and 'section'. How would I write the code for this?

            And thinking in a Java OO way, would it not be better to have 'M.course.format.swap_sections' not have an exception or else but rather call a function to return the 'sectionlistselector' which could then be overridden by a format if desired? Therefore the default functionality could be similar to:

            sectionlistselector = '.' + CSS.COURSECONTENT + ' li.' + CSS.SECTION;

            and overridden as:

            sectionlistselector = '.' + CSS.COURSECONTENT + ' ' + M.course.format.get_section_selector(Y);

            Or actually is this what 'M.course.format.get_section_selector' is sort of trying to do?

            And why do a lot of swaps have to occur when the old code simply did a high level swap on the container? As such:

                YAHOO.util.DDM.swapNode(this.getEl(), sectionIn.getEl());
                YAHOO.util.DDM.swapNode(this.getEl().previousSibling, sectionIn.getEl().previousSibling);

            This is surely far neater?

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - - edited Dan - no worries Dan and Andrew, TBH, I am now a little confused on the solution! I have two 'tr' tag containers containing the left, content and right css class 'td' tags. I need to swap them both at the same time. Each pair represents a single section. Their respective classes are 'cps' and 'section'. How would I write the code for this? And thinking in a Java OO way, would it not be better to have 'M.course.format.swap_sections' not have an exception or else but rather call a function to return the 'sectionlistselector' which could then be overridden by a format if desired? Therefore the default functionality could be similar to: sectionlistselector = '.' + CSS.COURSECONTENT + ' li.' + CSS.SECTION; and overridden as: sectionlistselector = '.' + CSS.COURSECONTENT + ' ' + M.course.format.get_section_selector(Y); Or actually is this what 'M.course.format.get_section_selector' is sort of trying to do? And why do a lot of swaps have to occur when the old code simply did a high level swap on the container? As such: YAHOO.util.DDM.swapNode(this.getEl(), sectionIn.getEl()); YAHOO.util.DDM.swapNode(this.getEl().previousSibling, sectionIn.getEl().previousSibling); This is surely far neater? Cheers, Gareth
            Hide
            kabalin Ruslan Kabalin added a comment -

            Dan: You are probably right about exceptions, your point is not using them at all as you mentioned on jabber, e.g. no default behaviour at all (since there is no particular reason to consider topics format as default, you are right) - the section dragdrop will work if and only if form.js is defined in format.

            Gareth: it looks like moving several 'tr' nodes (that form a section) is a problem, as we can't apply drag-drop handler to the group of nodes... any chance you could rewrite your format to use divs? That would be very helpful.

            And why do a lot of swaps have to occur when the old code simply did a high level swap on the container?

            We can't simply swap the container for a simple reason (in fact, in the old code, swapping container only was not sufficient either): we need to reorder containers and modify related id's and parameters that contain the section number. Say if you drag down section 1 down and insert in after section 3, you will have the order:
            2
            3
            1
            4
            5
            ...
            Now if you will try adding new resource, for example, using section 1 (which is third in the order), the actual resource will be added to section 2 (which now the first). The same applied to various titles, e.g. weeks title in week format and section number in topics format. If you know the better way how to make the swap more efficient, feel free to suggest it

            Show
            kabalin Ruslan Kabalin added a comment - Dan: You are probably right about exceptions, your point is not using them at all as you mentioned on jabber, e.g. no default behaviour at all (since there is no particular reason to consider topics format as default, you are right) - the section dragdrop will work if and only if form.js is defined in format. Gareth: it looks like moving several 'tr' nodes (that form a section) is a problem, as we can't apply drag-drop handler to the group of nodes... any chance you could rewrite your format to use divs? That would be very helpful. And why do a lot of swaps have to occur when the old code simply did a high level swap on the container? We can't simply swap the container for a simple reason (in fact, in the old code, swapping container only was not sufficient either): we need to reorder containers and modify related id's and parameters that contain the section number. Say if you drag down section 1 down and insert in after section 3, you will have the order: 2 3 1 4 5 ... Now if you will try adding new resource, for example, using section 1 (which is third in the order), the actual resource will be added to section 2 (which now the first). The same applied to various titles, e.g. weeks title in week format and section number in topics format. If you know the better way how to make the swap more efficient, feel free to suggest it
            Hide
            gb2048 Gareth J Barnard added a comment -

            @Ruslan: I just did a quick test and it is possible to encompass the two 'tr's within an enclosing single 'div' and things to still work. I would be willing to rewrite in the future to actually follow the topic's format 'li's but when I tried in the past I always succeeded in all browsers until I tested in Internet Explorer - then things went pear shaped - I'm sure you recognised the feeling. I have even read in the HTML 5 specification that tables should not be used for layouts - please tell that to Microsoft! As I struggle to make a simple CSS display attribute change in JavaScript without it messing up in IE. I think I've spent the same amount of time testing in IE as I have with all of the other browsers put together + development.

            TBH, I'm now getting a little out of my depth with this ATM as I'm more of a Java backend programmer by heart. However I'm willing to change anything with help .

            I'm also up to my eyeballs with this UK EU Cookie Law thing - CONTRIB-3624 - complete nightmare!

            So if you had an encompassing 'div' would that help?

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - @Ruslan: I just did a quick test and it is possible to encompass the two 'tr's within an enclosing single 'div' and things to still work. I would be willing to rewrite in the future to actually follow the topic's format 'li's but when I tried in the past I always succeeded in all browsers until I tested in Internet Explorer - then things went pear shaped - I'm sure you recognised the feeling. I have even read in the HTML 5 specification that tables should not be used for layouts - please tell that to Microsoft! As I struggle to make a simple CSS display attribute change in JavaScript without it messing up in IE. I think I've spent the same amount of time testing in IE as I have with all of the other browsers put together + development. TBH, I'm now getting a little out of my depth with this ATM as I'm more of a Java backend programmer by heart. However I'm willing to change anything with help . I'm also up to my eyeballs with this UK EU Cookie Law thing - CONTRIB-3624 - complete nightmare! So if you had an encompassing 'div' would that help? Cheers, Gareth
            Hide
            kabalin Ruslan Kabalin added a comment -

            @Gareth

            So if you had an encompassing 'div' would that help

            Using 'div' as child node of 'table' is against HTML specification as far as I can see, you may use 'tbody' for grouping purposes instead, as many times as you need

            Show
            kabalin Ruslan Kabalin added a comment - @Gareth So if you had an encompassing 'div' would that help Using 'div' as child node of 'table' is against HTML specification as far as I can see, you may use 'tbody' for grouping purposes instead, as many times as you need
            Hide
            gb2048 Gareth J Barnard added a comment -

            @Ruslan

            I've just tried with a repeating 'tbody' and no problem. I can put into my existing code and put in the 'master' branch of my format soon with the CONTRIB-3624 fix. Do you need a specific class to be associated with it? And will this work for you?

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - @Ruslan I've just tried with a repeating 'tbody' and no problem. I can put into my existing code and put in the 'master' branch of my format soon with the CONTRIB-3624 fix. Do you need a specific class to be associated with it? And will this work for you? Cheers, Gareth
            Hide
            kabalin Ruslan Kabalin added a comment -

            Do you need a specific class to be associated with it?

            Whatever class you like, the point here that M.course.format.get_section_selector should return 'tbody.someclass'.

            Show
            kabalin Ruslan Kabalin added a comment - Do you need a specific class to be associated with it? Whatever class you like, the point here that M.course.format.get_section_selector should return 'tbody.someclass'.
            Hide
            gb2048 Gareth J Barnard added a comment -

            @Ruslan

            Done, I've created a class 'topcollsection' - it will be in GitHub hopefully today.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - @Ruslan Done, I've created a class 'topcollsection' - it will be in GitHub hopefully today. Cheers, Gareth
            Hide
            kabalin Ruslan Kabalin added a comment -

            @Gareth: Please make sure it works with the patch attached to the bug

            Show
            kabalin Ruslan Kabalin added a comment - @Gareth: Please make sure it works with the patch attached to the bug
            Hide
            gb2048 Gareth J Barnard added a comment - - edited

            @Ruslan TBH I have no idea on how to do this! How do I apply a custom patch to a moodle installation and then trying with my code? As this is for 2.3, could I supply a version in my 'master' branch on GitHub and work through the changes?

            Show
            gb2048 Gareth J Barnard added a comment - - edited @Ruslan TBH I have no idea on how to do this! How do I apply a custom patch to a moodle installation and then trying with my code? As this is for 2.3, could I supply a version in my 'master' branch on GitHub and work through the changes?
            Hide
            gb2048 Gareth J Barnard added a comment - - edited

            @Ruslan Just twigged that I need to clone 'git://git.luns.net.uk/moodle.git' MDL-32657-master-1 branch, then I should integrate my code and create the 'format.js' file as in the 'weeks' format?

            Show
            gb2048 Gareth J Barnard added a comment - - edited @Ruslan Just twigged that I need to clone 'git://git.luns.net.uk/moodle.git' MDL-32657 -master-1 branch, then I should integrate my code and create the 'format.js' file as in the 'weeks' format?
            Hide
            kabalin Ruslan Kabalin added a comment -

            @Gareth: yes, that is what you need. But really, it is up to you, no rush at all, you can make your theme 2.3 compatible whenever you have time.

            Show
            kabalin Ruslan Kabalin added a comment - @Gareth: yes, that is what you need. But really, it is up to you, no rush at all, you can make your theme 2.3 compatible whenever you have time.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys, this has been integrated

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys, this has been integrated
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Oh can someone please add some testing instructions (just saw they were missing)

            Show
            samhemelryk Sam Hemelryk added a comment - Oh can someone please add some testing instructions (just saw they were missing)
            Hide
            kabalin Ruslan Kabalin added a comment -

            Oh no, did not notice it has been flagged as ready for integration... I have just written a patch to address Dan's comments

            Show
            kabalin Ruslan Kabalin added a comment - Oh no, did not notice it has been flagged as ready for integration... I have just written a patch to address Dan's comments
            Hide
            kabalin Ruslan Kabalin added a comment - - edited

            Added another patch that addresses Dan's comments and removes default behaviour: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=d95b77bdb74e9513243eb18c1b6b2f64685d5d95

            Show
            kabalin Ruslan Kabalin added a comment - - edited Added another patch that addresses Dan's comments and removes default behaviour: https://git.luns.net.uk/?p=moodle.git;a=commitdiff;h=d95b77bdb74e9513243eb18c1b6b2f64685d5d95
            Hide
            poltawski Dan Poltawski added a comment -

            Have pulled this into integration in the interests of expedeincy and testing effort.

            Show
            poltawski Dan Poltawski added a comment - Have pulled this into integration in the interests of expedeincy and testing effort.
            Hide
            kabalin Ruslan Kabalin added a comment -

            Thanks Dan, much appreciated.

            Show
            kabalin Ruslan Kabalin added a comment - Thanks Dan, much appreciated.
            Hide
            gb2048 Gareth J Barnard added a comment - - edited

            Dear all,

            I have a version of 2.3 from Luns - master-1 branch for this tracker. I've added 'format.js' and 'tbody' with a class of 'topcollsection' but despite having 'callback_topcoll_ajax_support()' in the 'lib.php' file the AJAX icon does not appear so I cant test further. I have turned off my 'section classes' overrides.

            Code available from https://github.com/gjb2048/moodle-format_topcoll/tree/MDL-32657 - this will install and be selectable as a course format. Full uninstall instructions in the Readme.txt file.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - - edited Dear all, I have a version of 2.3 from Luns - master-1 branch for this tracker. I've added 'format.js' and 'tbody' with a class of 'topcollsection' but despite having 'callback_topcoll_ajax_support()' in the 'lib.php' file the AJAX icon does not appear so I cant test further. I have turned off my 'section classes' overrides. Code available from https://github.com/gjb2048/moodle-format_topcoll/tree/MDL-32657 - this will install and be selectable as a course format. Full uninstall instructions in the Readme.txt file. Cheers, Gareth
            Hide
            gb2048 Gareth J Barnard added a comment -

            I think the issue is to do with:

                        // Initialise sections dragging
                        try {
                            this.sectionlistselector = '.'+CSS.COURSECONTENT+' '+M.course.format.get_section_selector(Y);
                        } catch (e) {
                            this.sectionlistselector = '.'+CSS.COURSECONTENT+' li.'+CSS.SECTION;
                        }
                        this.setup_for_section(this.sectionlistselector);

            in 'dragdrop.js' failing to pick up the correct selector. I've tried 'tr.togglesection' instead as that is the same more precise row but no joy.

            Show
            gb2048 Gareth J Barnard added a comment - I think the issue is to do with: // Initialise sections dragging try { this.sectionlistselector = '.'+CSS.COURSECONTENT+' '+M.course.format.get_section_selector(Y); } catch (e) { this.sectionlistselector = '.'+CSS.COURSECONTENT+' li.'+CSS.SECTION; } this.setup_for_section(this.sectionlistselector); in 'dragdrop.js' failing to pick up the correct selector. I've tried 'tr.togglesection' instead as that is the same more precise row but no joy.
            Hide
            gb2048 Gareth J Barnard added a comment -

            And 'tr.togglesection' has the section id for (in dragdrop.js):

                    get_section_id : function(node) {
                        return Number(node.get('id').replace(/section-/i, ''));
                    },

            Show
            gb2048 Gareth J Barnard added a comment - And 'tr.togglesection' has the section id for (in dragdrop.js): get_section_id : function(node) { return Number(node.get('id').replace(/section-/i, '')); },
            Hide
            kabalin Ruslan Kabalin added a comment -

            Gareth: the code you mentioned is not there any more, see last commit in https://git.luns.net.uk/moodle.git/shortlog/refs/heads/MDL-32657-master-2 which has been integrated now.

            Show
            kabalin Ruslan Kabalin added a comment - Gareth: the code you mentioned is not there any more, see last commit in https://git.luns.net.uk/moodle.git/shortlog/refs/heads/MDL-32657-master-2 which has been integrated now.
            Hide
            gb2048 Gareth J Barnard added a comment -

            So does that mean that 'Pull Master Branch' has to change above? What version should I now test against please?

            Show
            gb2048 Gareth J Barnard added a comment - So does that mean that 'Pull Master Branch' has to change above? What version should I now test against please?
            Hide
            gb2048 Gareth J Barnard added a comment -

            Latest version on https://github.com/gjb2048/moodle-format_topcoll/tree/MDL-32657

            Looking at the way dragdrop.js works I don't think it can be done unless I can overload almost the whole file or rewrite my entire format to use 'ul' and 'li' tags - which pragmatically could be restrictive in the variety of different formats contributors are able to produce to enrich Moodle. Thoughts?

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Latest version on https://github.com/gjb2048/moodle-format_topcoll/tree/MDL-32657 Looking at the way dragdrop.js works I don't think it can be done unless I can overload almost the whole file or rewrite my entire format to use 'ul' and 'li' tags - which pragmatically could be restrictive in the variety of different formats contributors are able to produce to enrich Moodle. Thoughts? Cheers, Gareth
            Hide
            gb2048 Gareth J Barnard added a comment -

            Guys,

            My code on GitHub now has all of the changes you wanted me to make, but still not working against the master-2 branch Ruslan has mentioned above.

            I have attempted different ways of getting my 'format.js' to work with 'dragdrop.js' in a polymophic way but to no avail. I've been looking at trying to use 'http://yuilibrary.com/yui/docs/node/' as a way of selecting the previous sibing for the preceeding tr.cps, but that does not work.

            Humm, what next?

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Guys, My code on GitHub now has all of the changes you wanted me to make, but still not working against the master-2 branch Ruslan has mentioned above. I have attempted different ways of getting my 'format.js' to work with 'dragdrop.js' in a polymophic way but to no avail. I've been looking at trying to use 'http://yuilibrary.com/yui/docs/node/' as a way of selecting the previous sibing for the preceeding tr.cps, but that does not work. Humm, what next? Gareth
            Hide
            gb2048 Gareth J Barnard added a comment -

            Question:

            In dragdrop.js you have 'var sectionlist = Y.Node.all(this.sectionlistselector);' and yet for each iteration of the bubble sort you have in 'format.js' 'var sectionlist = Y.Node.all('.'CSS.COURSECONTENT' '+M.course.format.get_section_selector(Y));' which appears to be the same thing and then only operate on two of the nodes returned (the number of sections in the course). Is this not really inefficient? Would it not be better to pass in the 'sectionlist' from 'dragdrop.js' into the called function of 'M.course.format.swap_sections'?

            Show
            gb2048 Gareth J Barnard added a comment - Question: In dragdrop.js you have 'var sectionlist = Y.Node.all(this.sectionlistselector);' and yet for each iteration of the bubble sort you have in 'format.js' 'var sectionlist = Y.Node.all('.' CSS.COURSECONTENT ' '+M.course.format.get_section_selector(Y));' which appears to be the same thing and then only operate on two of the nodes returned (the number of sections in the course). Is this not really inefficient? Would it not be better to pass in the 'sectionlist' from 'dragdrop.js' into the called function of 'M.course.format.swap_sections'?
            Hide
            gb2048 Gareth J Barnard added a comment -

            I see from 'http://yuilibrary.com/forum/viewtopic.php?p=26390' that the NodeList is a static snapshot representation of the DOM - so possibly why it has to work that way - but if a swap is made in the list then that is the correct order of things before committing back to the DOM?

            I've also looked at 'http://yuilibrary.com/forum/viewtopic.php?p=30522#p30522' where a table is being used instead of 'ul' and 'li' as hard coded in 'dragdrop.js' would operating with an enclosing 'div' within the enclosing 'tbody' be ok? And the standard formats have an enclosing 'div' for the left, content and right components?

            Show
            gb2048 Gareth J Barnard added a comment - I see from 'http://yuilibrary.com/forum/viewtopic.php?p=26390' that the NodeList is a static snapshot representation of the DOM - so possibly why it has to work that way - but if a swap is made in the list then that is the correct order of things before committing back to the DOM? I've also looked at 'http://yuilibrary.com/forum/viewtopic.php?p=30522#p30522' where a table is being used instead of 'ul' and 'li' as hard coded in 'dragdrop.js' would operating with an enclosing 'div' within the enclosing 'tbody' be ok? And the standard formats have an enclosing 'div' for the left, content and right components?
            Hide
            rwijaya Rossiani Wijaya added a comment -

            This is working great.

            Thanks Dan for integrationg the latest patch.

            Test passed.

            Show
            rwijaya Rossiani Wijaya added a comment - This is working great. Thanks Dan for integrationg the latest patch. Test passed.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            UPDATE tracker_issues
               SET status = 'Closed',
                  comment = 'Thanks!'
            WHEN participants = 'Did a gorgeous work'

            This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - UPDATE tracker_issues SET status = 'Closed', comment = 'Thanks!' WHEN participants = 'Did a gorgeous work' This landed upstream some hours ago (some - me - developer fell slept in the sofa yesterday before spamming this).
            Hide
            gb2048 Gareth J Barnard added a comment -

            Dear all,

            Umm, please assist me as I thought this was to facilitate the ability of non-core formats to support drag and drop after the removal of section classes. My recent comments appear to have been totally ignored.

            I'm sure that you did good work but consider that you missed the true point of the issue.

            Please can this issue be reopened so that changes can be put in place to support my format for the benefit of the users. I will adjust my code in anyway suitable.

            UPDATE tracker_issues
               SET status = 'Reopened',
                  comment = 'Please'
            WHEN participants = 'Did a gorgeous work but forgot to complete everything originally intended'
            WHERE tracker_id = 'MDL-32657'

            Thanks,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Dear all, Umm, please assist me as I thought this was to facilitate the ability of non-core formats to support drag and drop after the removal of section classes. My recent comments appear to have been totally ignored. I'm sure that you did good work but consider that you missed the true point of the issue. Please can this issue be reopened so that changes can be put in place to support my format for the benefit of the users. I will adjust my code in anyway suitable. UPDATE tracker_issues SET status = 'Reopened', comment = 'Please' WHEN participants = 'Did a gorgeous work but forgot to complete everything originally intended' WHERE tracker_id = 'MDL-32657' Thanks, Gareth
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Hi Gareth,

            We've actually been working pretty hard to get this working for you. This particular issue related to moving the swap logic to the course format rather than keeping it in the core javascript to enable course formats to make additional swaps. This particular piece of work is now closed and the patches have been integrated. All other course formats can (and have) been adjusted to work with the new javascript and we've almost got it working for yours.

            The complication is caused by several things:

            • your use of multiple table rows - we've nearly got this sorted by using a <tbody>
            • your use of multiple 'section' classes for things which aren't actually sections

            As I say, we've nearly go these all working but we may have some more to do. Ruslan is actively working on this and there may be other changes required which we'll be creating new issues for. We may also be able to provide a patch to update your course format.

            Regards,

            Andrew

            Show
            dobedobedoh Andrew Nicols added a comment - Hi Gareth, We've actually been working pretty hard to get this working for you. This particular issue related to moving the swap logic to the course format rather than keeping it in the core javascript to enable course formats to make additional swaps. This particular piece of work is now closed and the patches have been integrated. All other course formats can (and have) been adjusted to work with the new javascript and we've almost got it working for yours. The complication is caused by several things: your use of multiple table rows - we've nearly got this sorted by using a <tbody> your use of multiple 'section' classes for things which aren't actually sections As I say, we've nearly go these all working but we may have some more to do. Ruslan is actively working on this and there may be other changes required which we'll be creating new issues for. We may also be able to provide a patch to update your course format. Regards, Andrew
            Hide
            gb2048 Gareth J Barnard added a comment -

            Dear Andrew et al,

            I must apologise if I have been out of order. I really do appreciate what you do and the help that you give. It is perhaps a point of performing detached development away from the same physical environment that leads to misunderstandings - I've experienced both in my time and hence can see how easily misunderstandings can take hold.

            I can eliminate the multiple 'section' classes as all I need to do is have a 'section' class for the section row and a 'cps' class for the toggle row.

            I'm quite happy to quickly answer things like 'Can you change this to this?'.

            I've been thinking hard and there are several scenarios:

            1. Drop core AJAX support in my format and implement section_classes.js if the user has advanced features.
            2. As 1 but implement my own 'dragdrop.js'.
            3. Rewrite to use 'li's with a 'section' class as per core formats and 'divs' for the toggle / content. The disadvantage is the toggle no longer spans the entire width of the section but has the left and right columns on the sides.
            4. In 'dragdrop.js' allow polymorphism for 'drag_start' and 'drop_hit' to pass in custom tag's / classes for correct selection.

            Please let me know if there is anything I can do to help

            Thanks,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Dear Andrew et al, I must apologise if I have been out of order. I really do appreciate what you do and the help that you give. It is perhaps a point of performing detached development away from the same physical environment that leads to misunderstandings - I've experienced both in my time and hence can see how easily misunderstandings can take hold. I can eliminate the multiple 'section' classes as all I need to do is have a 'section' class for the section row and a 'cps' class for the toggle row. I'm quite happy to quickly answer things like 'Can you change this to this?'. I've been thinking hard and there are several scenarios: 1. Drop core AJAX support in my format and implement section_classes.js if the user has advanced features. 2. As 1 but implement my own 'dragdrop.js'. 3. Rewrite to use 'li's with a 'section' class as per core formats and 'divs' for the toggle / content. The disadvantage is the toggle no longer spans the entire width of the section but has the left and right columns on the sides. 4. In 'dragdrop.js' allow polymorphism for 'drag_start' and 'drop_hit' to pass in custom tag's / classes for correct selection. Please let me know if there is anything I can do to help Thanks, Gareth
            Hide
            gb2048 Gareth J Barnard added a comment -

            I've removed the 'section' classes from things that are not sections as requested. Updated code on 'https://github.com/gjb2048/moodle-format_topcoll/tree/MDL-32657'.

            Show
            gb2048 Gareth J Barnard added a comment - I've removed the 'section' classes from things that are not sections as requested. Updated code on 'https://github.com/gjb2048/moodle-format_topcoll/tree/MDL-32657'.
            Hide
            kabalin Ruslan Kabalin added a comment -

            Gareth: small surprise for you in MDL-32744

            Show
            kabalin Ruslan Kabalin added a comment - Gareth: small surprise for you in MDL-32744

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  25/Jun/12