Moodle
  1. Moodle
  2. MDL-36990

feedback module use drag and drop

    Details

    • Testing Instructions:
      Hide
      1. activate the feedback module on "Site administration"
      2. go into a course and create a feedback instance with each type of questions
      3. on each question now should be a drag icon. The legacy move icons should be disappeared.
      4. move the questions up or down and check the new position
      5. now disable the javascript in your browser
      6. move the items in the classic way by clicking on the move icons
      Show
      activate the feedback module on "Site administration" go into a course and create a feedback instance with each type of questions on each question now should be a drag icon. The legacy move icons should be disappeared. move the questions up or down and check the new position now disable the javascript in your browser move the items in the classic way by clicking on the move icons
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36990_master-neu
    • Rank:
      46526

      Description

      The feedback module should use drag and drop to order the question items on the editing page.

      1. feedback.ogv
        1.00 MB
        Rajesh Taneja

        Activity

        Hide
        Andreas Grabs added a comment -

        Hi,
        I integrated drag and drop into the feedback module. I use yui 3. It is my first try so there can be something incorrect.
        Andreas

        Show
        Andreas Grabs added a comment - Hi, I integrated drag and drop into the feedback module. I use yui 3. It is my first try so there can be something incorrect. Andreas
        Hide
        Michael de Raadt added a comment -

        Sounds like a good idea. Hopefully we can get this peer reviewed soon. I'm still not sure what the progress is on the new Survey module.

        Show
        Michael de Raadt added a comment - Sounds like a good idea. Hopefully we can get this peer reviewed soon. I'm still not sure what the progress is on the new Survey module.
        Hide
        Andreas Grabs added a comment -

        Hi Michael,
        Thank you for your comment. But I think it is a useful and needed improvement for this module regardless of the current state.
        Andreas

        Show
        Andreas Grabs added a comment - Hi Michael, Thank you for your comment. But I think it is a useful and needed improvement for this module regardless of the current state. Andreas
        Hide
        Damyon Wiese added a comment -

        Thanks Andreas,

        Just taking a look at this patch - note this won't be integrated until master and 2.4 diverge early next year - but we can get it ready now.

        First - I like the patch - the drag and drop works very nicely and is a good improvement.

        I did find one regression with this change (I'll attach a screenshot). With javascript disabled, the target boxes for the move function are appearing inside the content box for each question instead of between them. I confirmed that with the old code, the boxes did appear between the questions correctly. If you can address this when making the other changes listed below I'll be happy to review this again.

        [N] Syntax - The coding style was all very good - I only found some minor issues with comments, etc. (I'll list these separately below)
        [Y] Output
        [Y] Whitespace
        [Y] Language
        [-] Databases
        [N] Testing - The testing instructions also need to test that it works properly with javascript disabled.
        [N] Security - Improper use of confirm_sesskey
        [-] Documentation (I'll add a label as this is a ui change)
        [N] Git - The git commit messages need to include the component name after the tracker code. e.g. "MDL-36990 Feedback: item-position text change on dragdrop"
        [Y] Sanity check

        Issues:

        1. All comments should start with a capital letter and end with the correct punctuation mark (.?! etc). This includes comments in the javascript module.
        2. confirm_sesskey returns a boolean, so confirm_sesskey(); is not a useful statement. You should use require_sesskey(). (in mod/feedback/ajax.php)
        3. It is best not to mix functions and code in the same file (ajax.php). feedback_ajax_saveitemorder() should be moved to lib.php
        4. mod/feedback/ajax.php is missing the license comment and the doc block comment for the file.
        5. Strings should use single quotes by default unless variable substitution would make them more readable.
        6. You should not need to pass $yuibase, $ajaxscript and $httpswwwroot as parameters to mod_feedback_init (And this code would not work with CDN hosted YUI) - $yuibase and $httpswwroot are not even used and $ajaxscript should just be (M.cfg.wwwroot + '/mod/feedback/ajax.php')
        7. module.js - variable named MOVEICON should be lowercase.
        8. module.js line 183: Missing semicolon.

        Thanks again Andreas for the patch - I tried to provide as much feedback as possible to reduce the number of times this issue has to go back and forth. If you let me know when you have updated your branches - I'll be happy to re-review.

        Regards, Damyon

        Show
        Damyon Wiese added a comment - Thanks Andreas, Just taking a look at this patch - note this won't be integrated until master and 2.4 diverge early next year - but we can get it ready now. First - I like the patch - the drag and drop works very nicely and is a good improvement. I did find one regression with this change (I'll attach a screenshot). With javascript disabled, the target boxes for the move function are appearing inside the content box for each question instead of between them. I confirmed that with the old code, the boxes did appear between the questions correctly. If you can address this when making the other changes listed below I'll be happy to review this again. [N] Syntax - The coding style was all very good - I only found some minor issues with comments, etc. (I'll list these separately below) [Y] Output [Y] Whitespace [Y] Language [-] Databases [N] Testing - The testing instructions also need to test that it works properly with javascript disabled. [N] Security - Improper use of confirm_sesskey [-] Documentation (I'll add a label as this is a ui change) [N] Git - The git commit messages need to include the component name after the tracker code. e.g. " MDL-36990 Feedback: item-position text change on dragdrop" [Y] Sanity check Issues: All comments should start with a capital letter and end with the correct punctuation mark (.?! etc). This includes comments in the javascript module. confirm_sesskey returns a boolean, so confirm_sesskey(); is not a useful statement. You should use require_sesskey(). (in mod/feedback/ajax.php) It is best not to mix functions and code in the same file (ajax.php). feedback_ajax_saveitemorder() should be moved to lib.php mod/feedback/ajax.php is missing the license comment and the doc block comment for the file. Strings should use single quotes by default unless variable substitution would make them more readable. You should not need to pass $yuibase, $ajaxscript and $httpswwwroot as parameters to mod_feedback_init (And this code would not work with CDN hosted YUI) - $yuibase and $httpswwroot are not even used and $ajaxscript should just be (M.cfg.wwwroot + '/mod/feedback/ajax.php') module.js - variable named MOVEICON should be lowercase. module.js line 183: Missing semicolon. Thanks again Andreas for the patch - I tried to provide as much feedback as possible to reduce the number of times this issue has to go back and forth. If you let me know when you have updated your branches - I'll be happy to re-review. Regards, Damyon
        Hide
        Andreas Grabs added a comment -

        Hi Damyon,

        thank you very much for your detailed feedback.
        I fixed all mentioned things in the current branch.

        Best regards
        Andreas

        Show
        Andreas Grabs added a comment - Hi Damyon, thank you very much for your detailed feedback. I fixed all mentioned things in the current branch. Best regards Andreas
        Hide
        Damyon Wiese added a comment -

        Hi Andreas,

        The patches all look good. I tested this in IE8 and IE9 and found a javascript error.

        In mod/feedback/module.js line 143 -

                        success: function(transactionid, xhr) {
                            var response = xhr.response;
        

        should be:

                            var response = xhr.responseText;
        

        In order to be cross browser compliant. (An example is in repository/filepicker.js)

        Can you please add this fix to your branches?

        Thanks, Damyon

        Show
        Damyon Wiese added a comment - Hi Andreas, The patches all look good. I tested this in IE8 and IE9 and found a javascript error. In mod/feedback/module.js line 143 - success: function(transactionid, xhr) { var response = xhr.response; should be: var response = xhr.responseText; In order to be cross browser compliant. (An example is in repository/filepicker.js) Can you please add this fix to your branches? Thanks, Damyon
        Hide
        Andreas Grabs added a comment -

        Hi Damyon,

        thank you for this info! I've changed it. And sorry for the long duration.

        Best regards
        Andreas

        Show
        Andreas Grabs added a comment - Hi Damyon, thank you for this info! I've changed it. And sorry for the long duration. Best regards Andreas
        Hide
        Andrew Nicols added a comment - - edited

        Hi Andreas,

        Thanks for working on this :_

        I've just seen that this is up for peer review again. I know that Damyon is currently down as peer reviewer, but I have a few additional comments.

        YUI module or js_init_call

        This is more a personal preference than a requirement, but I'd rather this were implemented as a YUI module rather than using js_init_call. It shouldn't be difficult to migrate from one to the other in this case.

        In my opinion, YUI-style modules are easier to understand than this style largely due to their inherent structure it suggests.

        At present there are functions within the init function which makes it a little difficult to read.

        Moving to a YUI module would mean clearer access to the Y variable (no need to pass it in as an argument). The id argument would also be come an attribute against the M.mod_feedback object you're creating so you'd access it using this.get('cmid').

        There is a small cost associated with using Y.Base to create a YUI module, but it's negligible for a single instance such as this.

        Code issues

        mod/feedback/edit.php

        This whole section would be simpler if it were written as a YUI-style module

        • no need to pass the sesskey() - it's available as M.cfg.sesskey in JS

        mod/feedback/module.js

        • Using a constant to contain the CSS classes and selectors would be clearer and more maintainable in the future.
        • Line 108 - 116: The code to remove the legacy icons can be rewritten more clearly:
          Y.Node.all('span.feedback_item_command_moveup').each(function(v, k) {
              v.remove();
          });;
          

          Could be rewritten more clearly as:

          Y.all('span.feedback_item_command_moveup').remove();
          
        • Line 89: You may want to look at Y.DD.Delegate rather than Y.DD.Drag as it should be more efficient and reduce some code complexity (see https://github.com/jmvedrine/moodle-qtype_ddmatch/blob/master/yui/dragdrop/dragdrop.js for an example)
        • Line 89: Need to use DD groups to prevent this DD conflicting with other DD code in Moodle which may appear on the same page (e.g. block drag/drop)
        • Line 4 - 81: Moving the Y.DD.DDM.on() functions to their own functions would make for clearer reading of the code
        • Line 41: Rather than setting opacity, we should add a class which contains the opacity otherwise themers get grumpy (they can't override the themes)
        • Line 149: An error should be displayed if there was a failure
        Show
        Andrew Nicols added a comment - - edited Hi Andreas, Thanks for working on this :_ I've just seen that this is up for peer review again. I know that Damyon is currently down as peer reviewer, but I have a few additional comments. YUI module or js_init_call This is more a personal preference than a requirement, but I'd rather this were implemented as a YUI module rather than using js_init_call. It shouldn't be difficult to migrate from one to the other in this case. In my opinion, YUI-style modules are easier to understand than this style largely due to their inherent structure it suggests. At present there are functions within the init function which makes it a little difficult to read. Moving to a YUI module would mean clearer access to the Y variable (no need to pass it in as an argument). The id argument would also be come an attribute against the M.mod_feedback object you're creating so you'd access it using this.get('cmid'). There is a small cost associated with using Y.Base to create a YUI module, but it's negligible for a single instance such as this. Code issues mod/feedback/edit.php This whole section would be simpler if it were written as a YUI-style module no need to pass the sesskey() - it's available as M.cfg.sesskey in JS mod/feedback/module.js Using a constant to contain the CSS classes and selectors would be clearer and more maintainable in the future. Line 108 - 116: The code to remove the legacy icons can be rewritten more clearly: Y.Node.all('span.feedback_item_command_moveup').each(function(v, k) { v.remove(); });; Could be rewritten more clearly as: Y.all('span.feedback_item_command_moveup').remove(); Line 89: You may want to look at Y.DD.Delegate rather than Y.DD.Drag as it should be more efficient and reduce some code complexity (see https://github.com/jmvedrine/moodle-qtype_ddmatch/blob/master/yui/dragdrop/dragdrop.js for an example) Line 89: Need to use DD groups to prevent this DD conflicting with other DD code in Moodle which may appear on the same page (e.g. block drag/drop) Line 4 - 81: Moving the Y.DD.DDM.on() functions to their own functions would make for clearer reading of the code Line 41: Rather than setting opacity, we should add a class which contains the opacity otherwise themers get grumpy (they can't override the themes) Line 149: An error should be displayed if there was a failure
        Hide
        Andreas Grabs added a comment -

        Hi Andrew,

        thank you very much for these detailed informations.
        Except the delegation I've changed all points you have listed.
        With delegation I have actually some problems .
        But I will try more soon.

        Best regards
        Andreas

        Show
        Andreas Grabs added a comment - Hi Andrew, thank you very much for these detailed informations. Except the delegation I've changed all points you have listed. With delegation I have actually some problems . But I will try more soon. Best regards Andreas
        Hide
        Andreas Grabs added a comment - - edited

        Hi Andrew,

        I am ready .
        I have solved all points from your list.

        1. It's now a YUI-style module
        2. Now I use M.cfg.sesskey
        3. I use a constant containing the CSS (like course/yui/dragdrop)
        4. I remove the legacy icons in the way you described
        5. I use Y.DD.Delegate rather than Y.DD.Drag
        6. I use DD groups to prevent this DD conflicting with other DD code
        7. I moved the Y.DD.DDM.on() functions to their own functions
        8. I added a class which contains the opacity
        9. An error is displayed if there was a failure

        I had some problems with rebasing. I couldn't find the reason. So I have created a new clean branch and applied my commits.
        The new branch is "MDL-36990_master-neu". Sorry if it makes more effort for you.

        Best regards
        Andreas

        Show
        Andreas Grabs added a comment - - edited Hi Andrew, I am ready . I have solved all points from your list. It's now a YUI-style module Now I use M.cfg.sesskey I use a constant containing the CSS (like course/yui/dragdrop) I remove the legacy icons in the way you described I use Y.DD.Delegate rather than Y.DD.Drag I use DD groups to prevent this DD conflicting with other DD code I moved the Y.DD.DDM.on() functions to their own functions I added a class which contains the opacity An error is displayed if there was a failure I had some problems with rebasing. I couldn't find the reason. So I have created a new clean branch and applied my commits. The new branch is " MDL-36990 _master-neu". Sorry if it makes more effort for you. Best regards Andreas
        Hide
        Damyon Wiese added a comment - - edited

        Thanks Andreas for the updated patch:

        [W] Syntax (There is 1 commented out line of code in /mod/feedback/yui/dragdrop/dragdrop.js)
        [Y] Output
        [W] Whitespace (Inline comments should have a space after //)
        [Y] Language
        [Y] Databases
        [Y] Testing
        [Y] Security
        [-] Documentation
        [W] Git - There are a lot of commits in this branch and most of the early commits are missing the component name in the message. I'll squash them before sending for integration.
        [Y] Sanity check

        The comments are only a minor issue - I'll add a patch to remove the commented code just to save a round trip.

        Thanks again, Damyon

        Show
        Damyon Wiese added a comment - - edited Thanks Andreas for the updated patch: [W] Syntax (There is 1 commented out line of code in /mod/feedback/yui/dragdrop/dragdrop.js) [Y] Output [W] Whitespace (Inline comments should have a space after //) [Y] Language [Y] Databases [Y] Testing [Y] Security [-] Documentation [W] Git - There are a lot of commits in this branch and most of the early commits are missing the component name in the message. I'll squash them before sending for integration. [Y] Sanity check The comments are only a minor issue - I'll add a patch to remove the commented code just to save a round trip. Thanks again, Damyon
        Hide
        Damyon Wiese added a comment -

        Ready for integration review.

        Show
        Damyon Wiese added a comment - Ready for integration review.
        Hide
        Damyon Wiese added a comment -

        Actually - this patch is missing one thing (I'll recheck this properly tomorrow) but I don't think it checks to see if the user has disabled drag and drop (which is important for accessibility).

        Show
        Damyon Wiese added a comment - Actually - this patch is missing one thing (I'll recheck this properly tomorrow) but I don't think it checks to see if the user has disabled drag and drop (which is important for accessibility).
        Hide
        Andreas Grabs added a comment -

        Hi Damyon,

        thank you for this hint. Now the edit.php checks for $CFG->enableajax.
        But I am unsure a bit because the git is changed by you. Can you get my changes in this case?

        Andreas

        Show
        Andreas Grabs added a comment - Hi Damyon, thank you for this hint. Now the edit.php checks for $CFG->enableajax. But I am unsure a bit because the git is changed by you. Can you get my changes in this case? Andreas
        Hide
        Damyon Wiese added a comment -

        Yes - I'll pick your changes onto my branch and send for integration.

        Show
        Damyon Wiese added a comment - Yes - I'll pick your changes onto my branch and send for integration.
        Hide
        Andreas Grabs added a comment -

        Hi,
        I'd like to ask what the progress is.
        Are there something missing yet?
        Best regards
        Andreas

        Show
        Andreas Grabs added a comment - Hi, I'd like to ask what the progress is. Are there something missing yet? Best regards Andreas
        Hide
        Andreas Grabs added a comment -

        Hi, can someone tell me whether this is going to integrated? It is ready for over one month.
        Are there some missing things yet I have to solve so let me know.

        Show
        Andreas Grabs added a comment - Hi, can someone tell me whether this is going to integrated? It is ready for over one month. Are there some missing things yet I have to solve so let me know.
        Hide
        Andreas Grabs added a comment -

        Hi,
        I put the last diff url because the one from Damyon seemed to be outdated.
        Best regards
        Andreas

        Show
        Andreas Grabs added a comment - Hi, I put the last diff url because the one from Damyon seemed to be outdated. Best regards Andreas
        Hide
        Aparup Banerjee added a comment -

        Thank you! Its been integrated into master now.

        It was great to follow the commits along in parallel with all the great review suggestions.
        ps: i've tweaked styles.css for tabs that snuck in.

        Show
        Aparup Banerjee added a comment - Thank you! Its been integrated into master now. It was great to follow the commits along in parallel with all the great review suggestions. ps: i've tweaked styles.css for tabs that snuck in.
        Hide
        Rajesh Taneja added a comment -

        Sorry Andreas, I am failing this test because while moving question, question text starts scrolling right (see attached feedback.ogv)

        Rest all works fine.

        Few more observations:

        1. With one question you see move icon (which should not be there), try drag and leave it anywhere will give dropmiss error dialog (This happens with multiple questions as well.)
        2. Move this question icon (with disable js), is visible for single question feedback (Same issue in stable)
        3. Multiple question show up/down and move icon, probably we should get rid of up/down icons (Same issue in stable)
        Show
        Rajesh Taneja added a comment - Sorry Andreas, I am failing this test because while moving question, question text starts scrolling right (see attached feedback.ogv) Rest all works fine. Few more observations: With one question you see move icon (which should not be there), try drag and leave it anywhere will give dropmiss error dialog (This happens with multiple questions as well.) Move this question icon (with disable js), is visible for single question feedback (Same issue in stable) Multiple question show up/down and move icon, probably we should get rid of up/down icons (Same issue in stable)
        Hide
        Aparup Banerjee added a comment -

        nice catch Raj , i can't replicate the test scrolling (on ff or safari on mac here) but yes that should be fixed.

        the dropmiss trace dialog is interesting - a yui feature? (no idea)

        Show
        Aparup Banerjee added a comment - nice catch Raj , i can't replicate the test scrolling (on ff or safari on mac here) but yes that should be fixed. the dropmiss trace dialog is interesting - a yui feature? (no idea)
        Hide
        Andreas Grabs added a comment -

        Hi Rajesh,

        thank you for testing it.
        The scrolling to the right site is comming from the yui itself. It also happens while moving course sections if there are no blocks on the right site.
        At the moment I don't know what I can do to solve this.
        The other mentioned points I will check shortly.
        Best regards
        Andreas

        Show
        Andreas Grabs added a comment - Hi Rajesh, thank you for testing it. The scrolling to the right site is comming from the yui itself. It also happens while moving course sections if there are no blocks on the right site. At the moment I don't know what I can do to solve this. The other mentioned points I will check shortly. Best regards Andreas
        Hide
        Aparup Banerjee added a comment -

        The scrolling issue was also seen in MDL-33994
        i think this could be passed seeing that its master only and is a major improvement.

        Andreas, since the scrolling is an existing issue (MDL-33994 was closed won't fix), lets see if we can get a quick fix in here to handle 'dropmiss'. If not, i suggest new MDLs to fix them up.

        Show
        Aparup Banerjee added a comment - The scrolling issue was also seen in MDL-33994 i think this could be passed seeing that its master only and is a major improvement. Andreas, since the scrolling is an existing issue ( MDL-33994 was closed won't fix), lets see if we can get a quick fix in here to handle 'dropmiss'. If not, i suggest new MDLs to fix them up.
        Hide
        Andreas Grabs added a comment -

        Hi Rajesh and Aparup,
        I could just solve all issues .
        The scrolling problem I could solve by bounding my drag area to the 95% width.
        The other issues are solved too.
        Best regards
        Andreas

        Show
        Andreas Grabs added a comment - Hi Rajesh and Aparup, I could just solve all issues . The scrolling problem I could solve by bounding my drag area to the 95% width. The other issues are solved too. Best regards Andreas
        Hide
        Rajesh Taneja added a comment -

        Thanks Andreas,

        Aparup, can you please cherry-pick the last commit, so I can test this.

        Show
        Rajesh Taneja added a comment - Thanks Andreas, Aparup, can you please cherry-pick the last commit, so I can test this.
        Hide
        Aparup Banerjee added a comment -

        Thanks Andreas, all yours Raj.

        Show
        Aparup Banerjee added a comment - Thanks Andreas, all yours Raj.
        Hide
        Rajesh Taneja added a comment -

        Thanks for quick fix Andreas,

        Drag-n-drop works as expected.
        With one question in feedback, no move icon is visible (Both in JS or non-Js).

        NOTE: Either move or up/down icons should be present, but not both. We should consider removing up/down icons, and have similar behaviour as on my home page or activity movement on course page.

        Show
        Rajesh Taneja added a comment - Thanks for quick fix Andreas, Drag-n-drop works as expected. With one question in feedback, no move icon is visible (Both in JS or non-Js). NOTE: Either move or up/down icons should be present, but not both. We should consider removing up/down icons, and have similar behaviour as on my home page or activity movement on course page.
        Hide
        Andreas Grabs added a comment -

        Hi Rajesh,
        I am not sure that it would be an improvement to remove the up/down icons.
        There often is the requirement to switch the position of two questions and so you can do that with one single click.
        It was a request from the community years ago.
        In my opinion it shouldn't be removed. In addition these icons only will be shown if dragdrop is not active.
        So it should not be a big problem
        Best regards
        Andreas

        Show
        Andreas Grabs added a comment - Hi Rajesh, I am not sure that it would be an improvement to remove the up/down icons. There often is the requirement to switch the position of two questions and so you can do that with one single click. It was a request from the community years ago. In my opinion it shouldn't be removed. In addition these icons only will be shown if dragdrop is not active. So it should not be a big problem Best regards Andreas
        Hide
        Rajesh Taneja added a comment -

        Thanks Andreas,

        I was not aware of the history of these icons. It make sense to keep them after your explanation.

        Show
        Rajesh Taneja added a comment - Thanks Andreas, I was not aware of the history of these icons. It make sense to keep them after your explanation.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

        Thanks!

        PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

        Show
        Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks! PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: