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

Drag handle tooltip for sections is not updated

    Details

    • Testing Instructions:
      Hide
      1. Create/access a course with a topics format.
      2. Optionally, rename a section(s) with custom section names. (This I believe makes it easier to see the issue.)
      3. Add content to sections. (This I believe makes it easier to see the issue.)
      4. Move section(s) both down and up.
      5. Mouse over moved section.
      6. Notice tool tip is incorrect.
      7. Apply patch and purge all caches.
      8. Repeat 'Move sections' both down and up.
      9. Mouse over moved section.
      10. Notice tool tip to is correct.
      11. Repeat for a weeks format.
      Show
      Create/access a course with a topics format. Optionally, rename a section(s) with custom section names. (This I believe makes it easier to see the issue.) Add content to sections. (This I believe makes it easier to see the issue.) Move section(s) both down and up. Mouse over moved section. Notice tool tip is incorrect. Apply patch and purge all caches. Repeat 'Move sections' both down and up. Mouse over moved section. Notice tool tip to is correct. Repeat for a weeks format.
    • Workaround:
      Hide

      Refresh the page after moving the sections/topics.

      Show
      Refresh the page after moving the sections/topics.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-37901_master

      Description

      When re-ordering sections via drag and drop, the tooltip (EG: the alt attribute text) does not update. So, if I move section 5 above section 4, the tooltip for section 4 now says "Move section 5"

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              salvetore Michael de Raadt added a comment -

              I suspect this will be resolved by MDL-34798.

              Show
              salvetore Michael de Raadt added a comment - I suspect this will be resolved by MDL-34798 .
              Hide
              gb2048 Gareth J Barnard added a comment -

              Unfortunately not resolved by MDL-34798 . It is a separate issue. However, there is a solution using a swap function called by 'dragdrop.js', in 'format.js' of the course format, change:

              /**
               * Swap section
               *
               * @param {YUI} Y YUI3 instance
               * @param {string} node1 node to swap to
               * @param {string} node2 node to swap with
               * @return {NodeList} section list
               */
              M.course.format.swap_sections = function(Y, node1, node2) {
                  var CSS = {
                      COURSECONTENT : 'course-content',
                      SECTIONADDMENUS : 'section_add_menus'
                  };
               
                  var sectionlist = Y.Node.all('.'+CSS.COURSECONTENT+' '+M.course.format.get_section_selector(Y));
                  // Swap menus.
                  sectionlist.item(node1).one('.'+CSS.SECTIONADDMENUS).swap(sectionlist.item(node2).one('.'+CSS.SECTIONADDMENUS));
              }

              to

              /**
               * Swap section
               *
               * @param {YUI} Y YUI3 instance
               * @param {string} node1 node to swap to
               * @param {string} node2 node to swap with
               * @return {NodeList} section list
               */
              M.course.format.swap_sections = function(Y, node1, node2) {
                  var CSS = {
                      COURSECONTENT : 'course-content',
                      SECTIONADDMENUS : 'section_add_menus',
                      SECTIONLEFTSIDE : 'left'
                  };
               
                  var sectionlist = Y.Node.all('.'+CSS.COURSECONTENT+' '+M.course.format.get_section_selector(Y));
                  // Swap menus.
                  sectionlist.item(node1).one('.'+CSS.SECTIONADDMENUS).swap(sectionlist.item(node2).one('.'+CSS.SECTIONADDMENUS));
                  // Swap move icons.
                  sectionlist.item(node1).one('.'+CSS.SECTIONLEFTSIDE).swap(sectionlist.item(node2).one('.'+CSS.SECTIONLEFTSIDE));
              }

              Give me time, and I'll do it

              Show
              gb2048 Gareth J Barnard added a comment - Unfortunately not resolved by MDL-34798 . It is a separate issue. However, there is a solution using a swap function called by 'dragdrop.js', in 'format.js' of the course format, change: /** * Swap section * * @param {YUI} Y YUI3 instance * @param {string} node1 node to swap to * @param {string} node2 node to swap with * @return {NodeList} section list */ M.course.format.swap_sections = function(Y, node1, node2) { var CSS = { COURSECONTENT : 'course-content', SECTIONADDMENUS : 'section_add_menus' };   var sectionlist = Y.Node.all('.'+CSS.COURSECONTENT+' '+M.course.format.get_section_selector(Y)); // Swap menus. sectionlist.item(node1).one('.'+CSS.SECTIONADDMENUS).swap(sectionlist.item(node2).one('.'+CSS.SECTIONADDMENUS)); } to /** * Swap section * * @param {YUI} Y YUI3 instance * @param {string} node1 node to swap to * @param {string} node2 node to swap with * @return {NodeList} section list */ M.course.format.swap_sections = function(Y, node1, node2) { var CSS = { COURSECONTENT : 'course-content', SECTIONADDMENUS : 'section_add_menus', SECTIONLEFTSIDE : 'left' };   var sectionlist = Y.Node.all('.'+CSS.COURSECONTENT+' '+M.course.format.get_section_selector(Y)); // Swap menus. sectionlist.item(node1).one('.'+CSS.SECTIONADDMENUS).swap(sectionlist.item(node2).one('.'+CSS.SECTIONADDMENUS)); // Swap move icons. sectionlist.item(node1).one('.'+CSS.SECTIONLEFTSIDE).swap(sectionlist.item(node2).one('.'+CSS.SECTIONLEFTSIDE)); } Give me time, and I'll do it
              Hide
              gb2048 Gareth J Barnard added a comment -

              Dear Michael de Raadt

              If you reopen the issue, I'll provide fix branches etc.

              Cheers,

              Gareth

              Show
              gb2048 Gareth J Barnard added a comment - Dear Michael de Raadt If you reopen the issue, I'll provide fix branches etc. Cheers, Gareth
              Hide
              bushido Mark Nielsen added a comment -

              Thanks for the pointer Gareth! I'll try it out in our formats.

              Show
              bushido Mark Nielsen added a comment - Thanks for the pointer Gareth! I'll try it out in our formats.
              Hide
              salvetore Michael de Raadt added a comment -

              Thanks for keeping an eye on this, Gareth. I've reopened it for you.

              Show
              salvetore Michael de Raadt added a comment - Thanks for keeping an eye on this, Gareth. I've reopened it for you.
              Hide
              gb2048 Gareth J Barnard added a comment -

              Thanks to Michael for re-opening, I can now start working on the fix.

              Show
              gb2048 Gareth J Barnard added a comment - Thanks to Michael for re-opening, I can now start working on the fix.
              Hide
              gb2048 Gareth J Barnard added a comment -

              Trickier than I thought, as although the swap works, sometimes the numbers are not in sequence. Tricky. Need to think of a better solution.

              Show
              gb2048 Gareth J Barnard added a comment - Trickier than I thought, as although the swap works, sometimes the numbers are not in sequence. Tricky. Need to think of a better solution.
              Hide
              gb2048 Gareth J Barnard added a comment -

              Found another solution, in this example for Moodle 2.3:

              M.course.format.process_sections = function(Y, sectionlist, response, sectionfrom, sectionto) {
                  var CSS = {
                      SECTIONNAME : 'sectionname',
              		SECTIONLEFTSIDE : 'left .section-handle img'
                  };
               
                  if (response.action == 'move') {
                      // If moving up swap around 'sectionfrom' and 'sectionto' so the that loop operates.
                      if (sectionfrom > sectionto) {
                          var temp = sectionto;
                          sectionto = sectionfrom;
                          sectionfrom = temp;
                      }
              		
              		var ele;
              		var str;
              		var stridx;
              		var newstr;
              		
                      // Update titles in all affected sections.
                      for (var i = sectionfrom; i <= sectionto; i++) {
                          sectionlist.item(i).one('.'+CSS.SECTIONNAME).setContent(response.sectiontitles[i]);
              			ele = sectionlist.item(i).one('.'+CSS.SECTIONLEFTSIDE);
              			str = ele.getAttribute('alt');
              			stridx = str.lastIndexOf(' ');
              			newstr = str.substr(0, stridx +1) + i;
              			ele.setAttribute('alt', newstr);
              			ele.setAttribute('title', newstr);  // For FireFox as 'alt' is not refreshed.
                      }
                  }
              }

              Implemented version should be cleaner.

              Show
              gb2048 Gareth J Barnard added a comment - Found another solution, in this example for Moodle 2.3: M.course.format.process_sections = function(Y, sectionlist, response, sectionfrom, sectionto) { var CSS = { SECTIONNAME : 'sectionname', SECTIONLEFTSIDE : 'left .section-handle img' };   if (response.action == 'move') { // If moving up swap around 'sectionfrom' and 'sectionto' so the that loop operates. if (sectionfrom > sectionto) { var temp = sectionto; sectionto = sectionfrom; sectionfrom = temp; } var ele; var str; var stridx; var newstr; // Update titles in all affected sections. for (var i = sectionfrom; i <= sectionto; i++) { sectionlist.item(i).one('.'+CSS.SECTIONNAME).setContent(response.sectiontitles[i]); ele = sectionlist.item(i).one('.'+CSS.SECTIONLEFTSIDE); str = ele.getAttribute('alt'); stridx = str.lastIndexOf(' '); newstr = str.substr(0, stridx +1) + i; ele.setAttribute('alt', newstr); ele.setAttribute('title', newstr); // For FireFox as 'alt' is not refreshed. } } } Implemented version should be cleaner.
              Hide
              gb2048 Gareth J Barnard added a comment -

              Dear Michael de Raadt

              I've requested peer review, do I need to find somebody?

              Thanks,

              Gareth

              Show
              gb2048 Gareth J Barnard added a comment - Dear Michael de Raadt I've requested peer review, do I need to find somebody? Thanks, Gareth
              Hide
              salvetore Michael de Raadt added a comment -

              Hi, Gareth.

              Now that it is in this state, the issue will appear on the list of issues waiting for peer review. So, no, you don't actively have to find someone. It doesn't hurt to mention it on Dev chat, though.

              Show
              salvetore Michael de Raadt added a comment - Hi, Gareth. Now that it is in this state, the issue will appear on the list of issues waiting for peer review. So, no, you don't actively have to find someone. It doesn't hurt to mention it on Dev chat, though.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Ruslan,

              Would you be able to peer review this issue?

              Cheers,

              Andrew

              Show
              dobedobedoh Andrew Nicols added a comment - Ruslan, Would you be able to peer review this issue? Cheers, Andrew
              Hide
              kabalin Ruslan Kabalin added a comment -

              Added some github comments, please address them.

              Show
              kabalin Ruslan Kabalin added a comment - Added some github comments, please address them.
              Hide
              gb2048 Gareth J Barnard added a comment -

              Dear Ruslan,

              Thank you for reviewing the code. I've commented on two, please let me know if you can't see them.

              I cannot find your comment 'Did you test it when sectionfrom is higher than sectionto initially (when you move something up rather than down)?' though which was in my eMail to respond to? The sectionfrom being higher than sectionto when moving up test I fixed in MDL-34798 and implemented swapping code. So as the loop is forced to operate in one direction regardless of sectionfrom and sectionto implied direction, then yes it works. I did test when moving sections up as well as down.

              Kind regards,

              Gareth

              Show
              gb2048 Gareth J Barnard added a comment - Dear Ruslan, Thank you for reviewing the code. I've commented on two, please let me know if you can't see them. I cannot find your comment 'Did you test it when sectionfrom is higher than sectionto initially (when you move something up rather than down)?' though which was in my eMail to respond to? The sectionfrom being higher than sectionto when moving up test I fixed in MDL-34798 and implemented swapping code. So as the loop is forced to operate in one direction regardless of sectionfrom and sectionto implied direction, then yes it works. I did test when moving sections up as well as down. Kind regards, Gareth
              Hide
              gb2048 Gareth J Barnard added a comment -

              Dear Ruslan,

              All changes made as requested.

              All branches rebased, squashed and updated.

              Kind regards,

              Gareth

              Show
              gb2048 Gareth J Barnard added a comment - Dear Ruslan, All changes made as requested. All branches rebased, squashed and updated. Kind regards, Gareth
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Thanks Gareth,

              I'm submitting for integration now so hopefully this will get in next week.

              Show
              dobedobedoh Andrew Nicols added a comment - Thanks Gareth, I'm submitting for integration now so hopefully this will get in next week.
              Hide
              gb2048 Gareth J Barnard added a comment -

              Hopefully

              Cheers,

              Gareth

              Show
              gb2048 Gareth J Barnard added a comment - Hopefully Cheers, Gareth
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks guys, this has been integrated now.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
              Hide
              andyjdavis Andrew Davis added a comment -

              Works as described. Passing.

              Show
              andyjdavis Andrew Davis added a comment - Works as described. Passing.
              Hide
              stronk7 Eloy Lafuente (stronk7) added a comment -

              This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities.

              Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied).

              Thanks, closing as fixed!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - This is valid for unlimited entries to the, soon to be unveiled, Moodle Codebase Gardens. It includes free access to all facilities. Personal and non-transferable to all assignees, reviewers and testers in this issue. Valid until switching to Blackboard (100000€ penalization will be applied). Thanks, closing as fixed!

                People

                • Votes:
                  0 Vote for this issue
                  Watchers:
                  7 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    11/Mar/13