Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Forms Library
    • Labels:
    • Testing Instructions:
      Hide
      1. Log in as admin
      2. Go to course -> forum
      3. Add new discussion (note: For this test to happen the maximum number of attachments needs to be set to 1 so that dragging 2 files are one too many)
      4. Select two files on desktop and drag them to file area
      5. Color and text should change in file area while the mouse button is pressed
      6. Drop the files on file area
      7. Error message should be displayed, and one file will be attached.
      8. Delete the attached file and now drag and drop one file
      9. check for color and text on file area (it should be changed) and file should be attached.
      10. delete the file

      supplemental test: try to break thsi awesome dnd and report/comment about any issues.

      Show
      Log in as admin Go to course -> forum Add new discussion (note: For this test to happen the maximum number of attachments needs to be set to 1 so that dragging 2 files are one too many) Select two files on desktop and drag them to file area Color and text should change in file area while the mouse button is pressed Drop the files on file area Error message should be displayed, and one file will be attached. Delete the attached file and now drag and drop one file check for color and text on file area (it should be changed) and file should be attached. delete the file supplemental test: try to break thsi awesome dnd and report/comment about any issues.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-31114_dnd_ui_improvements
    • Rank:
      37543

      Description

      1. Currently Drag and drop show colour change, it will be nice to change text stating "Drop the files in this area" (Like gmail)
      2. After maximum file upload is reached (for file manager), DND event should show error message that you can't upload more then x files

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          It would probably be better if these two suggestions were separated into two issues.

          Show
          Michael de Raadt added a comment - It would probably be better if these two suggestions were separated into two issues.
          Hide
          Davo Smith added a comment -

          First item addressed here:
          https://github.com/davosmith/moodle/tree/MDL-31114_dnd_ui_improvements
          Diff:
          https://github.com/davosmith/moodle/commit/13064ab44bfcbf7a6d6bcf75efb7e03752fe97a6

          Note: I have had to add the 'uploadmessage' element into the FilePicker and FileManager code, as dynamically adding/removing the element as I dragged over the filepicker/filemanager was causing the overlay to occasionally get stuck on (I've not had this problem when showing/hiding the pre-existing overlay element).

          Show
          Davo Smith added a comment - First item addressed here: https://github.com/davosmith/moodle/tree/MDL-31114_dnd_ui_improvements Diff: https://github.com/davosmith/moodle/commit/13064ab44bfcbf7a6d6bcf75efb7e03752fe97a6 Note: I have had to add the 'uploadmessage' element into the FilePicker and FileManager code, as dynamically adding/removing the element as I dragged over the filepicker/filemanager was causing the overlay to occasionally get stuck on (I've not had this problem when showing/hiding the pre-existing overlay element).
          Hide
          Davo Smith added a comment -

          Just added a second commit to this same branch, to show an 'alert' message when the user attempts to upload past the 'maxfiles' limit. I note that there seems to be a bug in Chrome that is preventing it form displaying the 'spinner' when the alert box pops up. I'm not sure there is any way around this, as stepping through the code after setting a breakpoint causes it to work properly.

          Diff here:
          https://github.com/davosmith/moodle/commit/1dc2445a1190b9f8bc814119f99d002a285cfcc8

          Show
          Davo Smith added a comment - Just added a second commit to this same branch, to show an 'alert' message when the user attempts to upload past the 'maxfiles' limit. I note that there seems to be a bug in Chrome that is preventing it form displaying the 'spinner' when the alert box pops up. I'm not sure there is any way around this, as stepping through the code after setting a breakpoint causes it to work properly. Diff here: https://github.com/davosmith/moodle/commit/1dc2445a1190b9f8bc814119f99d002a285cfcc8
          Hide
          Sam Hemelryk added a comment -

          Hi Davo,

          I've been looking at this now.
          The changes look good, the only thing I noted was that we avoid using inline styles. In this case being JS output it doesn't matter too much just noting it.

          What I'm not sure about is the change in text.
          I'm in two minds about it, personally I think the change in colour is enough to make it clear that a drop action is possible. I can see the rational in a change like this I'm just not sure about the implementation.

          1. Having the text displayed only when the user drags a file over the drop area is a little redundant. I think to be useful we would need to display the text as soon as the user drags a file over the document. (Looked at how Gmail was doing to get an idea about what Raj was asking)
          2. Making the text look more like the underlying text would also help I feel, the shift from left aligned to center aligned is abrasive so far. Perhaps as well this won't be so bad in 1/ above is done.

          Let us know your thought Davo and Raj when Davo's had a chance to reply have a look and see what you think.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Davo, I've been looking at this now. The changes look good, the only thing I noted was that we avoid using inline styles. In this case being JS output it doesn't matter too much just noting it. What I'm not sure about is the change in text. I'm in two minds about it, personally I think the change in colour is enough to make it clear that a drop action is possible. I can see the rational in a change like this I'm just not sure about the implementation. Having the text displayed only when the user drags a file over the drop area is a little redundant. I think to be useful we would need to display the text as soon as the user drags a file over the document. (Looked at how Gmail was doing to get an idea about what Raj was asking) Making the text look more like the underlying text would also help I feel, the shift from left aligned to center aligned is abrasive so far. Perhaps as well this won't be so bad in 1/ above is done. Let us know your thought Davo and Raj when Davo's had a chance to reply have a look and see what you think. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Hello Sam and Davo,
          I agree with Sam here. I was hoping to see the old text initially and when user drag and drop the file, with change in color, there should be change in text. It might be redundant, but gives good user experience. I personally like the way gmail shows it, and probably helps in accessibility as well.

          On the other note, patch looks Great Davo.
          Thanks for working on this

          Show
          Rajesh Taneja added a comment - Hello Sam and Davo, I agree with Sam here. I was hoping to see the old text initially and when user drag and drop the file, with change in color, there should be change in text. It might be redundant, but gives good user experience. I personally like the way gmail shows it, and probably helps in accessibility as well. On the other note, patch looks Great Davo. Thanks for working on this
          Hide
          Davo Smith added a comment - - edited

          OK, so we're looking for:

          1. When the user drags a file onto the page, change the styling of the box (and / or text)
          2. When the user drags over the box, trigger a secondary change in the styling (and / or text)

          If someone wants to specify exactly what should be in the box (in terms of styling / text) in each of these situations, then I'll have a go at implementing it:

          a) No files uploaded already, drag (with files attached) enters page
          b) No files uploaded already, drag (with files attached) enters upload box
          c) Files already uploaded, drag (with files attached) enters page
          d) Files already uploaded, drag (with files attached) enters upload box

          Oh, and the inline styles I also normally avoid, but very occasionally, I slip them in (especially when they are more functional than 'design' related - without the 'position:relative' on the filemanager/picker box, the overlay fills the entire page, which seemed a bit too important to leave in a stylesheet somewhere - I could be convinced otherwise...).

          Show
          Davo Smith added a comment - - edited OK, so we're looking for: 1. When the user drags a file onto the page, change the styling of the box (and / or text) 2. When the user drags over the box, trigger a secondary change in the styling (and / or text) If someone wants to specify exactly what should be in the box (in terms of styling / text) in each of these situations, then I'll have a go at implementing it: a) No files uploaded already, drag (with files attached) enters page b) No files uploaded already, drag (with files attached) enters upload box c) Files already uploaded, drag (with files attached) enters page d) Files already uploaded, drag (with files attached) enters upload box Oh, and the inline styles I also normally avoid, but very occasionally, I slip them in (especially when they are more functional than 'design' related - without the 'position:relative' on the filemanager/picker box, the overlay fills the entire page, which seemed a bit too important to leave in a stylesheet somewhere - I could be convinced otherwise...).
          Hide
          Davo Smith added a comment -

          I've added another commit to the branch to show the 'drop here' message when you drag over the page and to highlight it when you drag over the correct element.

          Once Sam/Rajesh have had a chance to look over this, I'll do a bit of cherry-picking / rebasing to tidy up the commit history, before putting this forward for integration.

          Show
          Davo Smith added a comment - I've added another commit to the branch to show the 'drop here' message when you drag over the page and to highlight it when you drag over the correct element. Once Sam/Rajesh have had a chance to look over this, I'll do a bit of cherry-picking / rebasing to tidy up the commit history, before putting this forward for integration.
          Hide
          Davo Smith added a comment -

          Would anyone be able to give a bit more feedback on my latest patch?

          Show
          Davo Smith added a comment - Would anyone be able to give a bit more feedback on my latest patch?
          Hide
          Rajesh Taneja added a comment -

          Sorry for not looking at this Davo
          Patch looks spot-on.
          Pushing this up for integration review.

          Show
          Rajesh Taneja added a comment - Sorry for not looking at this Davo Patch looks spot-on. Pushing this up for integration review.
          Hide
          Davo Smith added a comment -

          Note there may be a clash with MDL-31321, which has just been integrated, as that makes an indentation change to almost all of the javascript file.

          If necessary, I'll reapply the changes from this issue, once this week's integrations are over.

          Show
          Davo Smith added a comment - Note there may be a clash with MDL-31321 , which has just been integrated, as that makes an indentation change to almost all of the javascript file. If necessary, I'll reapply the changes from this issue, once this week's integrations are over.
          Hide
          Aparup Banerjee added a comment -

          Hi Davo,
          i'm seeing conflicts with dndupload.js likely from another patch already integrated with changes to that file. (if MDL-31321 , i wish i had seen this issue first.) IF you can rebase (in a separate branch) against current integration.git that would be awesome.

          review still in prog...

          Show
          Aparup Banerjee added a comment - Hi Davo, i'm seeing conflicts with dndupload.js likely from another patch already integrated with changes to that file. (if MDL-31321 , i wish i had seen this issue first.) IF you can rebase (in a separate branch) against current integration.git that would be awesome. review still in prog...
          Hide
          Davo Smith added a comment -

          I've recreated the patches on top of integration master
          Seems to be working locally, so please feel free to attempt integration again.
          (Branch name / diff URL are the same)

          Show
          Davo Smith added a comment - I've recreated the patches on top of integration master Seems to be working locally, so please feel free to attempt integration again. (Branch name / diff URL are the same)
          Hide
          Aparup Banerjee added a comment -

          Thanks for that patch Davo it looks good (even with the so called nasty hack) and its tested fine for me as well (chrome/ubuntu).

          Thats been integrated into master only for any further testing.

          Tester: heres the challenge - try and break it the drag n drop in this test!

          Show
          Aparup Banerjee added a comment - Thanks for that patch Davo it looks good (even with the so called nasty hack) and its tested fine for me as well (chrome/ubuntu). Thats been integrated into master only for any further testing. Tester: heres the challenge - try and break it the drag n drop in this test!
          Hide
          Andrew Davis added a comment - - edited

          I have some general comments.

          In the help item next to "drag and drop available" it says "Note: this may not work with other web browsers" Other web browsers?

          Can we have "drop files here to upload" visible by default? And centred to. Having is disappear once the user has uploaded a single file is fine but having it hidden until the user drags a file into the page makes little sense. We're telling them drag and drop is available but not telling them where until they've dragged in a file.

          On a related note, the string "drag and drop available" above the file area should be removed. Telling the user that drag and drop is available somewhere on the page is unnecessarily mysterious. It would be better if the file area told the user that they could drag files there and if the area immediately above it, instead of saying "Maximum size for new files: 2MB - drag and drop available", said "Maximum size for new files: 2MB Maximum attachments: 2"

          "Error message should be displayed, and one file will be attached."
          Testing instructions are missing a small but important detail. For this to happen the maximum number of attachments needs to be set to 1.

          If the user drags in more files than than the maximum number of attachments attaching a random subset of their files isn't very helpful. If I am a teacher and I have 2 documents (say, the exam location announcement and the practice exam) to distribute to my students but I'm only allowed 1 attachment its unlikely that I'll say "ah well, that'll have to do" when only 1 of those documents attach while the other vanishes. We're just forcing the user to go through and delete any files that did upload before starting over ie putting those files into a zip then uploading that. We should probably either accept all of the files, or if we can't, then display an error and reject the whole lot.

          Should we hold this up for this or should I copy and paste my rant into a new MDL?

          Otherwise, it does indeed work fine.

          Show
          Andrew Davis added a comment - - edited I have some general comments. In the help item next to "drag and drop available" it says "Note: this may not work with other web browsers" Other web browsers? Can we have "drop files here to upload" visible by default? And centred to. Having is disappear once the user has uploaded a single file is fine but having it hidden until the user drags a file into the page makes little sense. We're telling them drag and drop is available but not telling them where until they've dragged in a file. On a related note, the string "drag and drop available" above the file area should be removed. Telling the user that drag and drop is available somewhere on the page is unnecessarily mysterious. It would be better if the file area told the user that they could drag files there and if the area immediately above it, instead of saying "Maximum size for new files: 2MB - drag and drop available", said "Maximum size for new files: 2MB Maximum attachments: 2" "Error message should be displayed, and one file will be attached." Testing instructions are missing a small but important detail. For this to happen the maximum number of attachments needs to be set to 1. If the user drags in more files than than the maximum number of attachments attaching a random subset of their files isn't very helpful. If I am a teacher and I have 2 documents (say, the exam location announcement and the practice exam) to distribute to my students but I'm only allowed 1 attachment its unlikely that I'll say "ah well, that'll have to do" when only 1 of those documents attach while the other vanishes. We're just forcing the user to go through and delete any files that did upload before starting over ie putting those files into a zip then uploading that. We should probably either accept all of the files, or if we can't, then display an error and reject the whole lot. Should we hold this up for this or should I copy and paste my rant into a new MDL? Otherwise, it does indeed work fine.
          Hide
          Andrew Davis added a comment -

          I have to go offline now for a few hours so putting this back in the testing pool.

          Show
          Andrew Davis added a comment - I have to go offline now for a few hours so putting this back in the testing pool.
          Hide
          Aparup Banerjee added a comment -

          +1 for 'reject the whole lot' of files vs random/1st file acceptance - however i would be tempted to let this pass and stay in master for some more feedback (noting that Davo likes feedback alot)

          i had 'For this to happen the maximum number of attachments needs to be set to 1.' by default but adding it to the test here.

          my +1 for a separate MDL to refine this further.

          Show
          Aparup Banerjee added a comment - +1 for 'reject the whole lot' of files vs random/1st file acceptance - however i would be tempted to let this pass and stay in master for some more feedback (noting that Davo likes feedback alot) i had 'For this to happen the maximum number of attachments needs to be set to 1.' by default but adding it to the test here. my +1 for a separate MDL to refine this further.
          Hide
          Rajesh Taneja added a comment -

          I agree with Aparup,
          my +1 to pass this and handle small issues in separate bug (or add more to MDL-31639)

          Show
          Rajesh Taneja added a comment - I agree with Aparup, my +1 to pass this and handle small issues in separate bug (or add more to MDL-31639 )
          Hide
          Davo Smith added a comment -

          Handling in a separate issue would be far easier for me, certainly.

          Rejecting all files when they would go over the limit is fine by me - I'll put together a patch that does that (very easy to do).

          How to tell the user that drag and drop is available (and what that means) is a wider issue, that I'd like a bit more feedback / consensus on. There's already been a bit of back & forth about this (changes to the text / help message, etc). I'll see if I can get any further comments about this.

          Show
          Davo Smith added a comment - Handling in a separate issue would be far easier for me, certainly. Rejecting all files when they would go over the limit is fine by me - I'll put together a patch that does that (very easy to do). How to tell the user that drag and drop is available (and what that means) is a wider issue, that I'd like a bit more feedback / consensus on. There's already been a bit of back & forth about this (changes to the text / help message, etc). I'll see if I can get any further comments about this.
          Hide
          Andrew Davis added a comment -

          Ok. Passing. I have opened 3 new MDLs. They're all linked. Hopefully we can break this into small pieces and get some consensus.

          Show
          Andrew Davis added a comment - Ok. Passing. I have opened 3 new MDLs. They're all linked. Hopefully we can break this into small pieces and get some consensus.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks!

          Closing as fixed, heading to zzzZZZzzz, niao

          Show
          Eloy Lafuente (stronk7) added a comment - It is late here and I'm very tired but I didn't want to go to sleep before expressing my admiration for your amazing collaboration. Thanks! Closing as fixed, heading to zzzZZZzzz, niao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: