Details

    • Testing Instructions:
      Hide
      • View a course with some activities/resources and at least two sections
      • Turn Editing on
      • Try each of the AJAX buttons on the resource(s):
        • move right
        • move left
        • drag/drop
        • show/hide
        • change group mode (on activities only)
      • Confirm:
        • the spinner appears at the end of activity toolbox (it does not replace the icon any more)
        • the spinner disappears after a short period
      • For a section try:
        • show/hide the section
        • highlight/unhighlight
        • drag/drop the section
      • Confirm:
        • a lightbox with a spinner centered in it appears over the section
        • the lightbox spinner disappears after a short period
      Show
      View a course with some activities/resources and at least two sections Turn Editing on Try each of the AJAX buttons on the resource(s): move right move left drag/drop show/hide change group mode (on activities only) Confirm: the spinner appears at the end of activity toolbox (it does not replace the icon any more) the spinner disappears after a short period For a section try: show/hide the section highlight/unhighlight drag/drop the section Confirm: a lightbox with a spinner centered in it appears over the section the lightbox spinner disappears after a short period
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32654-master-2
    • Rank:
      39591

      Description

      Another minor improvement following MDL-31052.

      Our initial idea to replace the icon with a spinner doesn't give great feedback and is easy to miss (hidden by the mousepointer).
      It also leads to a rather complicated set of events in the send_request function and I've also seen that clicking a button twice in quick succession can lead to the spinner not leaving.

      I've put together an alternative which simplifies send_requests and utilises the Node.show() and Node.hide() functions.
      I've also put together a function to create a spinner in M.util.add_spinner.

      I also noticed on the original implementation that if you click quickly enough then the spinner never disappears due to a race condition in the JS.

        Activity

        Hide
        Ruslan Kabalin added a comment -

        Looks good.

        Show
        Ruslan Kabalin added a comment - Looks good.
        Hide
        Ruslan Kabalin added a comment -

        While the code looks fine, some external opinion about this feature usability will be useful.

        Show
        Ruslan Kabalin added a comment - While the code looks fine, some external opinion about this feature usability will be useful.
        Hide
        Sam Hemelryk added a comment -

        Hi guys,

        I've just been looking over this now.
        The only thing I noted in code was that I think M.util.add_spinner should really be defining WAITICON.

        In general I think this code is an improvement, certainly its an improvement in simplicity.
        To be truthful I don't think the spinner is very visible, certainly I had to look for it when moving an activity.
        Perhaps in the future we should look for a way to make it more noticeable, either way at the moment I think this is a good improvement and I'm sure that it will be easier given now given these changes.

        I'll give you a bit of time before I pick this up again for integration to give you an opportunity to provide feedback.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, I've just been looking over this now. The only thing I noted in code was that I think M.util.add_spinner should really be defining WAITICON. In general I think this code is an improvement, certainly its an improvement in simplicity. To be truthful I don't think the spinner is very visible, certainly I had to look for it when moving an activity. Perhaps in the future we should look for a way to make it more noticeable, either way at the moment I think this is a good improvement and I'm sure that it will be easier given now given these changes. I'll give you a bit of time before I pick this up again for integration to give you an opportunity to provide feedback. Cheers Sam
        Hide
        Andrew Nicols added a comment -

        Hi Sam,

        I've added the WAITICON.

        I did have a play with a few other ways of displaying the icon but none seem ideal. I tried using the lightbox spinner (like for section changes) but it blocks further interaction because of the lightbox which isn't ideal.

        I've also increased the timeout before the spinner disappears. If the spinner has no timeout then it's so fast that it feels like it hasn't done anything. If it displays much longer then it makes it feel like the UI is sluggish.

        I was playing with the idea of setting a changing background to the whole line, but there's no easy way to make it themeable in YUI. In jQueryUI it could be done with Node.addClass('changing'), followed by Node.removeClass('changing', 500); to set the background (like we do for addspinner), and then gradually remove the class after it's been changed over 500 milliseconds.
        Sadly, it's not possible to add/remove classes with duration in YUI and without this it wouldn't be themeable.

        If you have any other thoughts, I'd be happy to have a good at implementing them.

        Andrew

        Show
        Andrew Nicols added a comment - Hi Sam, I've added the WAITICON. I did have a play with a few other ways of displaying the icon but none seem ideal. I tried using the lightbox spinner (like for section changes) but it blocks further interaction because of the lightbox which isn't ideal. I've also increased the timeout before the spinner disappears. If the spinner has no timeout then it's so fast that it feels like it hasn't done anything. If it displays much longer then it makes it feel like the UI is sluggish. I was playing with the idea of setting a changing background to the whole line, but there's no easy way to make it themeable in YUI. In jQueryUI it could be done with Node.addClass('changing'), followed by Node.removeClass('changing', 500); to set the background (like we do for addspinner), and then gradually remove the class after it's been changed over 500 milliseconds. Sadly, it's not possible to add/remove classes with duration in YUI and without this it wouldn't be themeable. If you have any other thoughts, I'd be happy to have a good at implementing them. Andrew
        Hide
        Sam Hemelryk added a comment -

        Thanks for the comment Andrew, and for tiding up WAITICON.
        Sounds like you've put a bit of effort into this and its good clarification to hear how you got to this solution. Certainly it sounds like the right one presently, doubly so given code freeze is just around the corner and this is a good improvement to land for 2.3.

        In regards to the idea of changing background colours, I'm not overly familiar with jQuery, however handling a duration for adding a class sounds like a very cool feature, especially in regards to things like this.
        In regards to YUI you could of course manually set up a timeout to do that, although perhaps what you are after is a transition which is something YUI supports, however I'm not sure they support background colour transitions.
        If they didn't it would be a very cool wee plugin to create.
        Certainly for the time being you've come up with a good solution!

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Thanks for the comment Andrew, and for tiding up WAITICON. Sounds like you've put a bit of effort into this and its good clarification to hear how you got to this solution. Certainly it sounds like the right one presently, doubly so given code freeze is just around the corner and this is a good improvement to land for 2.3. In regards to the idea of changing background colours, I'm not overly familiar with jQuery, however handling a duration for adding a class sounds like a very cool feature, especially in regards to things like this. In regards to YUI you could of course manually set up a timeout to do that, although perhaps what you are after is a transition which is something YUI supports, however I'm not sure they support background colour transitions. If they didn't it would be a very cool wee plugin to create. Certainly for the time being you've come up with a good solution! Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Found an issue while testing things. I was getting undefined now when chaning an activities indent, groupmode, or visibility.
        Problem was coming about because when creating the spinner it was attempting to do the following : Y.Node('li.activity').one('li.activity span.commands') which was returning null.
        Anyway I've integrated a fix for this now, can be seen with `git show b3c35188b`.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Found an issue while testing things. I was getting undefined now when chaning an activities indent, groupmode, or visibility. Problem was coming about because when creating the spinner it was attempting to do the following : Y.Node('li.activity').one('li.activity span.commands') which was returning null. Anyway I've integrated a fix for this now, can be seen with `git show b3c35188b`. Cheers Sam
        Hide
        Adrian Greeve added a comment -

        Spinners no longer replace the icons, but are instead at the side of either the activity / resource or the side of the topic.
        Thanks.

        Show
        Adrian Greeve added a comment - Spinners no longer replace the icons, but are instead at the side of either the activity / resource or the side of the topic. Thanks.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

        Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: