Details

    • Testing Instructions:
      Hide

      In as many browsers as you can muster:

      • Turn off a bunch of activities so that the potential size of the activity chooser list is smaller (at least smaller than your browser window)
      • Open a course page
      • Turn editing on
      • Ensure that your browser is as tall as possible
      • Open the Activity chooser
        • Confirm that the activity chooser does not show any excess whitespace at the bottom of the list of activities (compared to the screenshots)
      • Resize the window with the activity chooser still open
        • Confirm that the activity chooser resizes correctly
      Show
      In as many browsers as you can muster: Turn off a bunch of activities so that the potential size of the activity chooser list is smaller (at least smaller than your browser window) Open a course page Turn editing on Ensure that your browser is as tall as possible Open the Activity chooser Confirm that the activity chooser does not show any excess whitespace at the bottom of the list of activities (compared to the screenshots) Resize the window with the activity chooser still open Confirm that the activity chooser resizes correctly
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-37566-m24
    • Pull Master Branch:
    • Rank:
      47219

      Description

      There is empty space in activity chooser which should be removed
      See the linked issue for details and screen shot

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          This require JS, I reassign to andrew

          Show
          Jérôme Mouneyrac added a comment - This require JS, I reassign to andrew
          Hide
          Jérôme Mouneyrac added a comment - - edited

          Just to explain because it may be confusing with MDL-37566 marked as "discovered while testing" MDL-37194:
          MDL-37566 require JS to automatically resize the activity chooser following the size of the window and also the size of the activity list div. This is more tricky than the improvement I provided in MDL-37194. Note that what I've done in MDL-37194 is just to increase the minimum size of the activity chooser, resulting in my opinion in a way better UX than the previous state (no scroll bar). MDL-37566 was still standing before MDL-37194 - the tracker issue was just not existing.

          Show
          Jérôme Mouneyrac added a comment - - edited Just to explain because it may be confusing with MDL-37566 marked as "discovered while testing" MDL-37194 : MDL-37566 require JS to automatically resize the activity chooser following the size of the window and also the size of the activity list div. This is more tricky than the improvement I provided in MDL-37194 . Note that what I've done in MDL-37194 is just to increase the minimum size of the activity chooser, resulting in my opinion in a way better UX than the previous state (no scroll bar). MDL-37566 was still standing before MDL-37194 - the tracker issue was just not existing.
          Hide
          Andrew Nicols added a comment - - edited

          I discovered whilst testing this that it was using 'max-height' instead of 'maxHeight' on the setStyle calls - YUI (helpfully) uses camelCase for all CSS key names.
          The incorrect key name explains why I'd originally used height as well as max-height though.

          Show
          Andrew Nicols added a comment - - edited I discovered whilst testing this that it was using 'max-height' instead of 'maxHeight' on the setStyle calls - YUI (helpfully) uses camelCase for all CSS key names. The incorrect key name explains why I'd originally used height as well as max-height though.
          Hide
          Andrew Nicols added a comment -

          Will provide stable backfixes after peer review.

          Show
          Andrew Nicols added a comment - Will provide stable backfixes after peer review.
          Hide
          Frédéric Massart added a comment -

          Hi Andrew, this does make sense. Feel free to push for integration whenever you like. You might want to add the component to your commit message. And just for me, I am getting confused on what is supposed to become the base for any dialogue in Moodle, could you clarify that for me? Grepping moodle-dialogue I ended up on notifications... My thoughts were that maybe the patch would be worth being placed in a relevant parent (or moodle dialogue module).

          Many thanks!
          Fred

          Show
          Frédéric Massart added a comment - Hi Andrew, this does make sense. Feel free to push for integration whenever you like. You might want to add the component to your commit message. And just for me, I am getting confused on what is supposed to become the base for any dialogue in Moodle, could you clarify that for me? Grepping moodle-dialogue I ended up on notifications... My thoughts were that maybe the patch would be worth being placed in a relevant parent (or moodle dialogue module). Many thanks! Fred
          Hide
          Andrew Nicols added a comment -

          Thanks Fred,

          I've added the component to the commit.

          At present it's a bit all over the place with dialogues. We have a lot of dialogues in Moodle which simply extend Y.Widget or Y.Panel.
          Equally, we have an increasing amount extending M.core.dialogue which adds a couple of Moodle-specific things - e.g. the close button. Helpfully, M.core.dialogue is a class in the the moodle-core-notification module.

          This particular change is only relevant to chooser dialogues. However, I am hoping to extract this part of the chooser dialogue into a separate YUI module so that any dialogue in Moodle can make itself automagically resizable.

          So in summary, it's currently in the right place

          Show
          Andrew Nicols added a comment - Thanks Fred, I've added the component to the commit. At present it's a bit all over the place with dialogues. We have a lot of dialogues in Moodle which simply extend Y.Widget or Y.Panel. Equally, we have an increasing amount extending M.core.dialogue which adds a couple of Moodle-specific things - e.g. the close button. Helpfully, M.core.dialogue is a class in the the moodle-core-notification module. This particular change is only relevant to chooser dialogues. However, I am hoping to extract this part of the chooser dialogue into a separate YUI module so that any dialogue in Moodle can make itself automagically resizable. So in summary, it's currently in the right place
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I trust you, integrated (23, 24 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - I trust you, integrated (23, 24 & master), thanks!
          Hide
          Rajesh Taneja added a comment -

          Sorry guys, I am not sure if this is correct.
          If user has just one activity enabled (forum), click on activity chooser show:

          1. Activity chooser is not centred.
          2. Selecting activity show activity description in small scrollable window.

          AFAIR activity chooser should be in centre and description should be of min. size.

          Correct me if I am wrong. Attaching screenshot for reference.

          Show
          Rajesh Taneja added a comment - Sorry guys, I am not sure if this is correct. If user has just one activity enabled (forum), click on activity chooser show: Activity chooser is not centred. Selecting activity show activity description in small scrollable window. AFAIR activity chooser should be in centre and description should be of min. size. Correct me if I am wrong. Attaching screenshot for reference.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Yeah, quite small. Surely a min should be always observed, for readability. Well spotted!

          Show
          Eloy Lafuente (stronk7) added a comment - Yeah, quite small. Surely a min should be always observed, for readability. Well spotted!
          Hide
          Andrew Nicols added a comment -

          Darn.

          The centring is a difficult problem but I have a solution for it:

          • we now set the max-height rather than the height of the dialogue. This means that we don't get the empty whitespace at the bottom;
          • we used to calculate the top of the dialogue by taking the height of it (now max-height) from the window height, and dividing by two;
          • now that we use max-height, the actual height can change;
          • to fix the centring we need to get the height instead;
          • the only way to get the height is dialogue.getStyle('height'), but this return the style value, not the height in pixels. e.g. 400px;
          • there is no easy way to convert from string to integer here - whilst most browsers will return it using px, there could be a case where we get it in em. Converting between the two is a bit of a pain and depends on parents in the HTML hierarchy.

          Our best course of action is probably:

          • get the actual dialogue height;
          • if it has 'px$', remove that and convert to number - instant height and centring returns
          • if not, then fall back to the calculated maxheight and live with the funky offset. It will just be slightly too high.

          For the min-height issue though, that's actually much harder still. We were previously not specifying a min-height and purely specifying a height for the dialogue which effectively catered for the min-height option.
          If we do set a min-height that's fairly large (say 400px), then this affects how the dialogue is shown on screen when there isn't much vertical screen height - the dialogue min-height is larger than the minheight JS variable we use for calculating the dialogue height. There are situations where the min-height from the theme may make sense for the dialogue all the way up until the screen gets small.
          Again, we could parse the min-height and play around with things but I think that to do so would be wrong.

          The only sane thing I can think of is to have three settings - minheight, maxheight, and a new value for baseheight which would be the normal height to use right up until the browser window got too small.

          Show
          Andrew Nicols added a comment - Darn. The centring is a difficult problem but I have a solution for it: we now set the max-height rather than the height of the dialogue. This means that we don't get the empty whitespace at the bottom; we used to calculate the top of the dialogue by taking the height of it (now max-height) from the window height, and dividing by two; now that we use max-height, the actual height can change; to fix the centring we need to get the height instead; the only way to get the height is dialogue.getStyle('height'), but this return the style value, not the height in pixels. e.g. 400px; there is no easy way to convert from string to integer here - whilst most browsers will return it using px, there could be a case where we get it in em. Converting between the two is a bit of a pain and depends on parents in the HTML hierarchy. Our best course of action is probably: get the actual dialogue height; if it has 'px$', remove that and convert to number - instant height and centring returns if not, then fall back to the calculated maxheight and live with the funky offset. It will just be slightly too high. For the min-height issue though, that's actually much harder still. We were previously not specifying a min-height and purely specifying a height for the dialogue which effectively catered for the min-height option. If we do set a min-height that's fairly large (say 400px), then this affects how the dialogue is shown on screen when there isn't much vertical screen height - the dialogue min-height is larger than the minheight JS variable we use for calculating the dialogue height. There are situations where the min-height from the theme may make sense for the dialogue all the way up until the screen gets small. Again, we could parse the min-height and play around with things but I think that to do so would be wrong. The only sane thing I can think of is to have three settings - minheight, maxheight, and a new value for baseheight which would be the normal height to use right up until the browser window got too small.
          Hide
          Andrew Nicols added a comment -

          I've pushed a fix which deals with both of these issues.
          git://github.com/andrewnicols/moodle.git
          MDL-37566-m-integration
          https://github.com/andrewnicols/moodle/commit/MDL-37566-m-integration

          Show
          Andrew Nicols added a comment - I've pushed a fix which deals with both of these issues. git://github.com/andrewnicols/moodle.git MDL-37566 -m-integration https://github.com/andrewnicols/moodle/commit/MDL-37566-m-integration
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sent to integration (23, 24 & master). Note that, for stables, I got a conflict with one "var dialoguetop" being there, while master is not using the "var". I assumed that master was the more correct, so took it out from stables.

          http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=3c754bb4427b3b0a1621d396143c44f41215caad

          Sending back to testing... thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Sent to integration (23, 24 & master). Note that, for stables, I got a conflict with one "var dialoguetop" being there, while master is not using the "var". I assumed that master was the more correct, so took it out from stables. http://git.moodle.org/gw?p=integration.git;a=commitdiff;h=3c754bb4427b3b0a1621d396143c44f41215caad Sending back to testing... thanks!
          Hide
          Rajesh Taneja added a comment -

          Thanks for fixing this Andrew,

          Gap is visible, only if there are few activities/resources. (Which is fine)

          Awesome work guys

          Show
          Rajesh Taneja added a comment - Thanks for fixing this Andrew, Gap is visible, only if there are few activities/resources. (Which is fine) Awesome work guys
          Hide
          Damyon Wiese added a comment -

          Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone!

          Regards, Damyon

          Show
          Damyon Wiese added a comment - Congratulations! This issue has been resolved. Thanks for helping to make Moodle better for everyone! Regards, Damyon

            People

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

              Dates

              • Created:
                Updated:
                Resolved: