Moodle
  1. Moodle
  2. MDL-32657

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor 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
    • Rank:
      39597

      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.

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          Gareth J Barnard added a comment -

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

          Show
          Gareth J Barnard added a comment - I'm a muppet, just found 'course-content' css class. However my other questions still stand
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Gareth J Barnard added a comment -

          @Ruslan

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

          Cheers,

          Gareth

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

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

          Show
          Ruslan Kabalin added a comment - @Gareth: Please make sure it works with the patch attached to the bug
          Hide
          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
          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
          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
          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
          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
          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
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated

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

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

          Show
          Sam Hemelryk added a comment - Oh can someone please add some testing instructions (just saw they were missing)
          Hide
          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
          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
          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
          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
          Dan Poltawski added a comment -

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

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

          Thanks Dan, much appreciated.

          Show
          Ruslan Kabalin added a comment - Thanks Dan, much appreciated.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Rossiani Wijaya added a comment -

          This is working great.

          Thanks Dan for integrationg the latest patch.

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working great. Thanks Dan for integrationg the latest patch. Test passed.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Ruslan Kabalin added a comment -

          Gareth: small surprise for you in MDL-32744

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