Moodle
  1. Moodle
  2. MDL-37194

Activity chooser is never displayed entirely on base theme

    Details

    • Rank:
      46770

      Description

      With the base theme, the activity chooser never displays the full list of activity/resource even thought the window height is large enough.

      First screenshot: after fix
      Second screenshot: before fix

        Issue Links

          Activity

          Hide
          Jérôme Mouneyrac added a comment -

          As it's javascript related, I sent it to you Andrew for peer-review.

          Show
          Jérôme Mouneyrac added a comment - As it's javascript related, I sent it to you Andrew for peer-review.
          Hide
          Andrew Nicols added a comment -

          Hi Jerome,

          This should really be done at the chooserdialogue level rather than the module chooser. Additionally, you need to update the CSS for

          .jsenabled .choosercontainer #chooseform .instruction, .jsenabled .choosercontainer #chooseform .typesummary

          The activity chooser was originally chosen at this size for aesthetic reasons.

          Adding Barbara as a watcher for her design opinion.

          Andrew

          Show
          Andrew Nicols added a comment - Hi Jerome, This should really be done at the chooserdialogue level rather than the module chooser. Additionally, you need to update the CSS for .jsenabled .choosercontainer #chooseform .instruction, .jsenabled .choosercontainer #chooseform .typesummary The activity chooser was originally chosen at this size for aesthetic reasons. Adding Barbara as a watcher for her design opinion. Andrew
          Hide
          Andrew Nicols added a comment -

          On second thoughts, I can see why you've kept the parameter change in the modchooser.js. Still needs the CSS change though, and I'd still appreciate barbara's thoughts here.

          Andrew

          Show
          Andrew Nicols added a comment - On second thoughts, I can see why you've kept the parameter change in the modchooser.js. Still needs the CSS change though, and I'd still appreciate barbara's thoughts here. Andrew
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks for looking at it Andrew.

          In my opinion it's UX before aesthetic. I think users should not have to scroll when it's not necessary.

          Thanks for the CSS I have forgotten it in the patch.

          As you noticed I just increased the maximum size - it's currently fixed to a lower value. In my opinion having a maximum pixel size is wrong anyway. It already looks tiny on my computer, it will look even smaller on the futur ultra HD monitors/TVs. But this quick patch still being an improvement I thought I'll send it anyway

          About aesthetic - some personal taste:
          I would remove the top bar (title and cross). The user knows he's adding an activity, no need to take some space. In my opinion, it looks cleaner. The cross button is redundant with the Cancel button as the window is always on screen. The cross button has square corners which looks weird against the dialog rounded corners.

          I'll wait for Barbara opinions before doing any changes.

          Show
          Jérôme Mouneyrac added a comment - Thanks for looking at it Andrew. In my opinion it's UX before aesthetic. I think users should not have to scroll when it's not necessary. Thanks for the CSS I have forgotten it in the patch. As you noticed I just increased the maximum size - it's currently fixed to a lower value. In my opinion having a maximum pixel size is wrong anyway. It already looks tiny on my computer, it will look even smaller on the futur ultra HD monitors/TVs. But this quick patch still being an improvement I thought I'll send it anyway About aesthetic - some personal taste: I would remove the top bar (title and cross). The user knows he's adding an activity, no need to take some space. In my opinion, it looks cleaner. The cross button is redundant with the Cancel button as the window is always on screen. The cross button has square corners which looks weird against the dialog rounded corners. I'll wait for Barbara opinions before doing any changes.
          Hide
          Jérôme Mouneyrac added a comment -

          Changing the peer-review to Barbara so she can notice it.

          Show
          Jérôme Mouneyrac added a comment - Changing the peer-review to Barbara so she can notice it.
          Hide
          Jérôme Mouneyrac added a comment -

          I asked Barbara, she's ok. I'll check css you mention and I'll submit to integration. Thanks Andrew.

          Show
          Jérôme Mouneyrac added a comment - I asked Barbara, she's ok. I'll check css you mention and I'll submit to integration. Thanks Andrew.
          Hide
          Jérôme Mouneyrac added a comment -

          Submitting to integration. tested on IE8/9 and Chrome.

          Show
          Jérôme Mouneyrac added a comment - Submitting to integration. tested on IE8/9 and Chrome.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          Adding Mary Evans here as theme component maintainer.

          Show
          Dan Poltawski added a comment - Adding Mary Evans here as theme component maintainer.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 24 and 23 thanks Jerome.

          Show
          Dan Poltawski added a comment - Integrated to master, 24 and 23 thanks Jerome.
          Hide
          Mary Evans added a comment - - edited

          Please do be careful with the CSS in Base theme as it affects ALL the CORE themes one way or another, so be sure to test them all.

          Show
          Mary Evans added a comment - - edited Please do be careful with the CSS in Base theme as it affects ALL the CORE themes one way or another, so be sure to test them all.
          Hide
          Jérôme Mouneyrac added a comment -

          Thanks Mary. From reading your comment I read a "testing guide line" for the issue tester. Or did you find anything wrong?

          Note: I remember testing four/five themes and there were all good. The reason I didn't test all of the core theme is that I don't consider that this fix will impact them - The fix allows a bigger activity chooser window and the Moodle JS is in charge to resize it anyway.

          Show
          Jérôme Mouneyrac added a comment - Thanks Mary. From reading your comment I read a "testing guide line" for the issue tester. Or did you find anything wrong? Note: I remember testing four/five themes and there were all good. The reason I didn't test all of the core theme is that I don't consider that this fix will impact them - The fix allows a bigger activity chooser window and the Moodle JS is in charge to resize it anyway.
          Hide
          Ankit Agarwal added a comment -

          As seen on the attached pic , activity chooser in 23 has empty space, is that a bad thing?
          Thanks

          Show
          Ankit Agarwal added a comment - As seen on the attached pic , activity chooser in 23 has empty space, is that a bad thing? Thanks
          Hide
          Jérôme Mouneyrac added a comment -

          For me the current state of the patch is a better UX than having a scrolling bar. As you noticed this grey space and thought it was worth a comment, I would suggest that you open an new issue for it Obviously it didn't bug me, but now that you are mentioning I think like you that it would be better to have a window that fit the activity list.

          From now I would pass this improvement issue and create a new improvement issue about the activity chooser not fitting the activity list (which was already the case before, but at least now we see the entire list).

          Show
          Jérôme Mouneyrac added a comment - For me the current state of the patch is a better UX than having a scrolling bar. As you noticed this grey space and thought it was worth a comment, I would suggest that you open an new issue for it Obviously it didn't bug me, but now that you are mentioning I think like you that it would be better to have a window that fit the activity list. From now I would pass this improvement issue and create a new improvement issue about the activity chooser not fitting the activity list (which was already the case before, but at least now we see the entire list).
          Hide
          Ankit Agarwal added a comment -

          Passing based on the comments above, created and linked issue.
          Thanks

          Show
          Ankit Agarwal added a comment - Passing based on the comments above, created and linked issue. Thanks
          Hide
          Dan Poltawski added a comment -

          Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          Show
          Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: