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 2.4 Branch:
      wip-MDL-37901_M24
    • Pull Master Branch:
      wip-MDL-37901_master
    • Rank:
      47655

      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"

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          I suspect this will be resolved by MDL-34798.

          Show
          Michael de Raadt added a comment - I suspect this will be resolved by MDL-34798 .
          Hide
          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
          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
          Gareth J Barnard added a comment -

          Dear Michael de Raadt

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

          Cheers,

          Gareth

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

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

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

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

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

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

          Show
          Gareth J Barnard added a comment - Thanks to Michael for re-opening, I can now start working on the fix.
          Hide
          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
          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
          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
          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
          Gareth J Barnard added a comment -

          Dear Michael de Raadt

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

          Thanks,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Michael de Raadt I've requested peer review, do I need to find somebody? Thanks, Gareth
          Hide
          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
          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
          Andrew Nicols added a comment -

          Ruslan,

          Would you be able to peer review this issue?

          Cheers,

          Andrew

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

          Added some github comments, please address them.

          Show
          Ruslan Kabalin added a comment - Added some github comments, please address them.
          Hide
          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
          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
          Gareth J Barnard added a comment -

          Dear Ruslan,

          All changes made as requested.

          All branches rebased, squashed and updated.

          Kind regards,

          Gareth

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

          Thanks Gareth,

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

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

          Hopefully

          Cheers,

          Gareth

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

          Thanks guys, this has been integrated now.

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

          Works as described. Passing.

          Show
          Andrew Davis added a comment - Works as described. Passing.
          Hide
          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
          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: