Details

    • Testing Instructions:
      Hide
      1. With javascript enabled in one browser (your choice) visit some long/short/complicated/simple moodle forms and verify they function correctly (no specific instructions - give the forms a good work out).
        1. Some good forms to test:
          1. Quiz settings
          2. Workshop settings
          3. Choice settings
          4. Course settings
          5. Grade item settings
          6. Backup Course (all steps)
          7. Restore Course (all steps)
      2. In all supported browsers test the functionality of the new javascript shortened forms for the "Choice settings" form.
      3. In one browser, disable javascript and make sure that the forms are still usable (but not collapsible and missing "Show more.../Show less" links). (Test the "Quiz settings" form)

      Note for testers: (Assignment has a known bug which means the sections show as expanded by default - exclude this from the tests).

      Show
      With javascript enabled in one browser (your choice) visit some long/short/complicated/simple moodle forms and verify they function correctly (no specific instructions - give the forms a good work out). Some good forms to test: Quiz settings Workshop settings Choice settings Course settings Grade item settings Backup Course (all steps) Restore Course (all steps) In all supported browsers test the functionality of the new javascript shortened forms for the "Choice settings" form. In one browser, disable javascript and make sure that the forms are still usable (but not collapsible and missing "Show more.../Show less" links). (Test the "Quiz settings" form) Note for testers: (Assignment has a known bug which means the sections show as expanded by default - exclude this from the tests).
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-30637-master
    • Rank:
      33448

      Description

      As discussed in http://moodle.org/mod/forum/discuss.php?d=191549, we've simplified the forms making them easier to use and navigate.

      This enhancement works by:

      • always displaying the 'General' section and closing all others by default;
      • always displaying a section with a required element;
      • opening any section which contains validation errors;
      • opening any section which was previously open on previous submit (e.g. when adding new choices);
      • displaying elements which are coded as expanded by default (e.g. $mform->setExpanded('foo'); and
      • removing the 'Show Advanced' button (using JS).

      There are still a couple of minor issues that we're aware of and working to address:

      • repeated elements are not shown by default because none of them are required until they're submitted - we've fixed it for the 'choice' module, and we're working on extending it to all affected modules;

      From the positive comments we've received so far, we'd like to submit it for integration into master.

      1. advanced_mock_hideadv.jpg
        13 kB
      2. advanced_mock_showadv.jpg
        11 kB
      3. advanced_mock_showless.jpg
        12 kB
      4. advanced_mock_showmore.jpg
        10 kB
      5. padding.png
        21 kB
      6. shortforms.jpg
        65 kB

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Adding Martin, what do you think about this idea?

          Show
          Dan Poltawski added a comment - Adding Martin, what do you think about this idea?
          Hide
          Nadav Kavalerchik added a comment -

          I LOVE it

          I made a similar hack. see forum discussion :
          http://moodle.org/mod/forum/discuss.php?d=191768#p835765

          Show
          Nadav Kavalerchik added a comment - I LOVE it I made a similar hack. see forum discussion : http://moodle.org/mod/forum/discuss.php?d=191768#p835765
          Hide
          Ruslan Kabalin added a comment -

          I have added a functionality of disabling shortforms feature for particular forms using:

          $mform->setDisableShrtforms();

          Could be useful for the cases where shortforms are not approprate, e.g. when lesson activities are presented to student (as reported in the forum). The feature public branch MDL-uidemo1-master-1 has been updated as well.

          Show
          Ruslan Kabalin added a comment - I have added a functionality of disabling shortforms feature for particular forms using: $mform->setDisableShrtforms(); Could be useful for the cases where shortforms are not approprate, e.g. when lesson activities are presented to student (as reported in the forum ). The feature public branch MDL-uidemo1-master-1 has been updated as well.
          Hide
          Séverin Terrier added a comment -

          This could be useful

          But, to let everybody work the way he preferes, i would add these options :

          • the Administrator could choose to use this "simplified" form (as default for the whole site)
          • each user could choose in his profile to use this "simplified" form (or not)
          • add an expand all option

          Some people prefere one long page, with everything directly visible, avoiding multiple click, and allowing browser search

          Show
          Séverin Terrier added a comment - This could be useful But, to let everybody work the way he preferes, i would add these options : the Administrator could choose to use this "simplified" form (as default for the whole site) each user could choose in his profile to use this "simplified" form (or not) add an expand all option Some people prefere one long page, with everything directly visible, avoiding multiple click, and allowing browser search
          Hide
          Andrew Nicols added a comment -

          I like the idea of the 'expand all' button.

          I'm in two minds when it comes to adding additional settings though. On one hand, it gives users more power, but on the other it can make it more complicated for new users.

          Show
          Andrew Nicols added a comment - I like the idea of the 'expand all' button. I'm in two minds when it comes to adding additional settings though. On one hand, it gives users more power, but on the other it can make it more complicated for new users.
          Hide
          Przemyslaw Stencel added a comment -

          It's a very welcome development. Teachers new to moodle are often overwhelmed by all the options. I always tell them just to fill in the most important ones and (for a while) ignore all others.

          My personal opinion is that as many options as possible should be collapsed at first. And I'd be very radical here - e.g. in a forum or quiz form, I'd show only the fields which are absolutely necessary, that is name and description fields at first. All others should be collapsed, including opening/closing dates, time limit, etc.

          Thank you for working on this!

          Show
          Przemyslaw Stencel added a comment - It's a very welcome development. Teachers new to moodle are often overwhelmed by all the options. I always tell them just to fill in the most important ones and (for a while) ignore all others. My personal opinion is that as many options as possible should be collapsed at first. And I'd be very radical here - e.g. in a forum or quiz form, I'd show only the fields which are absolutely necessary, that is name and description fields at first. All others should be collapsed, including opening/closing dates, time limit, etc. Thank you for working on this!
          Hide
          Martin Dougiamas added a comment -

          I've not seen it in action yet but this has been floating around and I do really like this idea, although all the subtasks are required, to make sure forms only improve and don't get worse for any cases.

          +10 from me!

          Show
          Martin Dougiamas added a comment - I've not seen it in action yet but this has been floating around and I do really like this idea, although all the subtasks are required, to make sure forms only improve and don't get worse for any cases. +10 from me!
          Hide
          Nadav Kavalerchik added a comment - - edited

          And How about adding a default javascript setFocus to the first element on the form?
          (and maybe some CSS rule to highlight that selected HTML input field too?)

          I am sure it will increase usability

          Show
          Nadav Kavalerchik added a comment - - edited And How about adding a default javascript setFocus to the first element on the form? (and maybe some CSS rule to highlight that selected HTML input field too?) I am sure it will increase usability
          Hide
          Nadav Kavalerchik added a comment -

          @Ruslan , @Dan

          I have just pulled this patch (feature) from danpoltwski/luns-shortforms-rebased and rebased it on top of a fresh moodle.org/master (19-9-2012) and it seems it has some YUI (JS) issues

          Can you please rebase it on a recent master branch and see if you can dig out those issues, since I'd LOVE to tryout this feature on one of our development systems that is open for teachers. So I could get some review and feedback on this beautiful idea.

          Any chance you have some spare time for this?

          Show
          Nadav Kavalerchik added a comment - @Ruslan , @Dan I have just pulled this patch (feature) from danpoltwski/luns-shortforms-rebased and rebased it on top of a fresh moodle.org/master (19-9-2012) and it seems it has some YUI (JS) issues Can you please rebase it on a recent master branch and see if you can dig out those issues, since I'd LOVE to tryout this feature on one of our development systems that is open for teachers. So I could get some review and feedback on this beautiful idea. Any chance you have some spare time for this?
          Hide
          Martin Dougiamas added a comment -

          I definitely wsnt this in 2.4. Adding it to the board.

          Show
          Martin Dougiamas added a comment - I definitely wsnt this in 2.4. Adding it to the board.
          Hide
          Martin Dougiamas added a comment -

          Mudrak, can you also have a look at this?

          Show
          Martin Dougiamas added a comment - Mudrak, can you also have a look at this?
          Hide
          Dan Poltawski added a comment -

          Ruslan: Do you know if you've made any more changes to core forms to set them expanded since we originally created this patch? It might be useful.

          Show
          Dan Poltawski added a comment - Ruslan: Do you know if you've made any more changes to core forms to set them expanded since we originally created this patch? It might be useful.
          Hide
          Damyon Wiese added a comment -

          Thanks for everyone working on this - I think this is a great feature.

          I've gone through the functionality and have some suggestions:

          1. The feature adds shortened forms - not shortened menus. The language strings should reflect that.

          2. The help text says it is just for the course mod adding/editing page - but it applies all forms.

          3. A form with only one section should not really be collapsible (e.g. admin/tool/uploaduser/index.php)

          4. When a section is open - the toggle icon is hard against the section name. I think it needs a space.

          5. External Tool is still showing the "Show advanced" button (for me). I think it's because it's in the general section.

          6. It might be nice to expand the section when some form fields have non-default values. E.g. the tags section on a wiki
          page (We will probably find cases like this and can just fix each one)

          7. It might be nice to expand a section if the current user has previously visited a form of the same type and expanded the section (Cookie?)

          8. IMS Package upload form is missing a required field.

          Show
          Damyon Wiese added a comment - Thanks for everyone working on this - I think this is a great feature. I've gone through the functionality and have some suggestions: 1. The feature adds shortened forms - not shortened menus. The language strings should reflect that. 2. The help text says it is just for the course mod adding/editing page - but it applies all forms. 3. A form with only one section should not really be collapsible (e.g. admin/tool/uploaduser/index.php) 4. When a section is open - the toggle icon is hard against the section name. I think it needs a space. 5. External Tool is still showing the "Show advanced" button (for me). I think it's because it's in the general section. 6. It might be nice to expand the section when some form fields have non-default values. E.g. the tags section on a wiki page (We will probably find cases like this and can just fix each one) 7. It might be nice to expand a section if the current user has previously visited a form of the same type and expanded the section (Cookie?) 8. IMS Package upload form is missing a required field.
          Hide
          Tim Hunt added a comment -

          Thanks for taking this on Damyon. I am generally in favour of this change, but there is one bit that worries me.

          Are we really sure that we want to get rid of Show advanced?

          I am sure that once we have collapsible sections, we will decide to stop using show advanced in many forms, since the new mechanism works better. However, I think that some of our most complex forms could benefit from keeping Show advanced. I am thinking, in particular, of the quiz settings form. We have a nice feature where admins can decide which form fields are advanced, thereby optimising the form for their institution. I would be sorry to see that go.

          I suppose there are two answers to that:

          A. If we completely re-did forms lib, we could re-do that ability for admins to customise forms in a way that applies to any form. That would be cool, but is not going to happen any time soon.

          B. If we just go for collapsible settings, with no show advanced, then I can change the quiz form so that instead of 'Show advance', any settings the admin marks as advanced instead get moved to an 'Advanced' section at the end of the form. That would be almost as good, but would move those settings out of order, and the order of things on the quiz form attempts to be logical.

          Some other points, continuing your numbering:

          7. Possibly a good idea, but should use get/set_user_preference. (There is already an ajax API for that.)

          9. I think a form should be able to specify which sections should be collapsed. This should default to the first section un-collapsed, but for a few forms, that default would not be right. Two examples that come to mind:
          a. Question preview form (as it currently is in integration / next week's weekly).
          b. Question import/export forms.
          These are all two-section forms where neither section should be collapsible. I suggest an API like
          $mform->set_non_collapsible_sections(array $names)
          if you don't call that, it defaults to using just the first section in the form.

          I hope these comments are helpful.

          Show
          Tim Hunt added a comment - Thanks for taking this on Damyon. I am generally in favour of this change, but there is one bit that worries me. Are we really sure that we want to get rid of Show advanced? I am sure that once we have collapsible sections, we will decide to stop using show advanced in many forms, since the new mechanism works better. However, I think that some of our most complex forms could benefit from keeping Show advanced. I am thinking, in particular, of the quiz settings form. We have a nice feature where admins can decide which form fields are advanced, thereby optimising the form for their institution. I would be sorry to see that go. I suppose there are two answers to that: A. If we completely re-did forms lib, we could re-do that ability for admins to customise forms in a way that applies to any form. That would be cool, but is not going to happen any time soon. B. If we just go for collapsible settings, with no show advanced, then I can change the quiz form so that instead of 'Show advance', any settings the admin marks as advanced instead get moved to an 'Advanced' section at the end of the form. That would be almost as good, but would move those settings out of order, and the order of things on the quiz form attempts to be logical. Some other points, continuing your numbering: 7. Possibly a good idea, but should use get/set_user_preference. (There is already an ajax API for that.) 9. I think a form should be able to specify which sections should be collapsed. This should default to the first section un-collapsed, but for a few forms, that default would not be right. Two examples that come to mind: a. Question preview form (as it currently is in integration / next week's weekly). b. Question import/export forms. These are all two-section forms where neither section should be collapsible. I suggest an API like $mform->set_non_collapsible_sections(array $names) if you don't call that, it defaults to using just the first section in the form. I hope these comments are helpful.
          Hide
          Joseph Rézeau added a comment -

          Tim wrote "We have a nice feature where admins can decide which form fields are advanced, thereby optimising the form for their institution. I would be sorry to see that go."

          Well, it depends a) on the size of the institution and b) on the degree of computer-literacy of the institution's staff (teachers). In a large institution (such as a university) with teaching staff having a wide range of computer-literacy (ranging from nil to advanced), how can an admin decide a "one size fits all" setting? An impossible task.

          I would much prefer that such settings be enabled at user (i.e. teacher) level, not admin. But this is a more general wish from my part. I have consistently deplored that in Moodle too many settings are reserved for admins, which should be settable by teachers. In that respect, the Moodle model is still too rigid, not democratic enough.

          Show
          Joseph Rézeau added a comment - Tim wrote "We have a nice feature where admins can decide which form fields are advanced, thereby optimising the form for their institution. I would be sorry to see that go." Well, it depends a) on the size of the institution and b) on the degree of computer-literacy of the institution's staff (teachers). In a large institution (such as a university) with teaching staff having a wide range of computer-literacy (ranging from nil to advanced), how can an admin decide a "one size fits all" setting? An impossible task. I would much prefer that such settings be enabled at user (i.e. teacher) level, not admin. But this is a more general wish from my part. I have consistently deplored that in Moodle too many settings are reserved for admins, which should be settable by teachers. In that respect, the Moodle model is still too rigid, not democratic enough.
          Hide
          Dan Poltawski added a comment -

          Are we really sure that we want to get rid of Show advanced?

          I just can't think of a way where we could sensibly mix these two concepts to be confusing.

          A. If we completely re-did forms lib, we could re-do that ability for admins to customise forms in a way that applies to any form. That would be cool, but is not going to happen any time soon.

          Actually me and Damyon were talking about how he wants to do that in assignment too, and we have the same thing in the course settings too and i'm sure elsewhere. So we should consider if there is a fast/cheap way of doing this!

          7. Absolutely, we can't use cookie, must be user preference. But one concern I have for this is how we store and retrieve these preferences efficiently across all Moodle forms. Do we do this elsewhere (show advanced?)? Store some sort of json-type representation of the form state based on class name?

          9. I think a form should be able to specify which sections should be collapsed. [...]

          This is already in place, with $mform->setExpanded('sectionname'), from Ruslan's forum post outlining it, the rules are:

          • always displaying the 'General' section and closing all others by default;
          • always displaying a section with a required element;
          • opening any section which contains validation errors;
          • opening any section which was previously open on previous submit (e.g. when adding new choices);
          • displaying elements which are coded as expanded by default (e.g. $mform->setExpanded('foo'); and
          Show
          Dan Poltawski added a comment - Are we really sure that we want to get rid of Show advanced? I just can't think of a way where we could sensibly mix these two concepts to be confusing. A. If we completely re-did forms lib, we could re-do that ability for admins to customise forms in a way that applies to any form. That would be cool, but is not going to happen any time soon. Actually me and Damyon were talking about how he wants to do that in assignment too, and we have the same thing in the course settings too and i'm sure elsewhere. So we should consider if there is a fast/cheap way of doing this! 7. Absolutely, we can't use cookie, must be user preference. But one concern I have for this is how we store and retrieve these preferences efficiently across all Moodle forms. Do we do this elsewhere (show advanced?)? Store some sort of json-type representation of the form state based on class name? 9. I think a form should be able to specify which sections should be collapsed. [...] This is already in place, with $mform->setExpanded('sectionname'), from Ruslan's forum post outlining it, the rules are: always displaying the 'General' section and closing all others by default; always displaying a section with a required element; opening any section which contains validation errors; opening any section which was previously open on previous submit (e.g. when adding new choices); displaying elements which are coded as expanded by default (e.g. $mform->setExpanded('foo') ; and
          Hide
          Nadav Kavalerchik added a comment -

          Here is a link to a Hack I made years ago on Moodle 1.9.2 (because of many Teachers heavy request to simplify the UI. which was also mentioned by Josef, on the above previous comment) on Moodle 2+ I have not had the time to hack something similar and was hoping this Issue will gain momentum and get implemented in core.

          See it in action: http://www.youtube.com/watch?v=I9I-usZ2lsA

          And some snippets of code I dug up (hope I did not leave any important piece out)

          The actual hide/show javascript code:

          $mform->addElement('static', 'hidingjscode', '', '<script>
          function shownextchildclass(id) {
            if ( document.getElementById(id).parentNode.nextSibling.nextSibling.nextSibling.attributes[0].nodeValue == \'fcontainer clearfix\') {
              document.getElementById(id).parentNode.nextSibling.nextSibling.nextSibling.attributes[0].nodeValue = \'fcontainer clearfix hide\';
            } else {
              document.getElementById(id).parentNode.nextSibling.nextSibling.nextSibling.attributes[0].nodeValue = \'fcontainer clearfix\';
            }
            //alert(document.getElementById(\'id_toggel1\').parentNode.nextSibling.nextSibling.nextSibling.attributes[0].nodeValue);
          }
          
          function hideheaders() {
          document.getElementById(\'id_enrolmentsheader\').parentNode.parentNode.childNodes[4].attributes[0].nodeValue = \'fcontainer clearfix hide\';
          document.getElementById(\'id_expirynotifyhdrheader\').parentNode.parentNode.childNodes[4].attributes[0].nodeValue = \'fcontainer clearfix hide\';
          document.getElementById(\'id_rolerenamingheader\').parentNode.parentNode.childNodes[4].attributes[0].nodeValue = \'fcontainer clearfix hide\';
          }
          window.addEventListener(\'load\',hideheaders,false);
          
          </script>
          <style>
          .hide {
          display:none;
          }
          </style>
          ');
          

          And one of the onClick javascript function + image that invoke the hide/show

          //$mform->addElement('header','enrolhdr', get_string('enrolments'));
          $mform->addElement('header', 'enrolhdr', get_string('enrolments'). '  <img src="'.$CFG->wwwroot.'/mod/tab/pix/arrow-down-double.png" id="id_enrolmentsheader" onclick="shownextchildclass(\'id_enrolmentsheader\')">');
          

          Obviously, the above code is not a usage suggestion of any kind. just to show that the need is very old and that I had to work=a=round the current Form's library to hack my way so the teachers could use the system more easily. BTW, showing the teachers the option to "show in advanced" was not helpful enough.

          Most (80%) teachers (Basic Content Creators) would like to be able to change two...three important items on any Form and submit it. They are intimidated by even seeing all the options. (If we had more teachers in the tracker, they could verify this. I am sure)

          I am even suggesting the add two Tabs on the top of each Form, with "Basic" and "Advanced" titles. which will default to "Basic" in which it will display to the teachers two...three most important input fields (items).
          And if they are more experienced and advanced Teachers (Content Creators) they can switch view to the "Advanced" tab and fill out more settings. ( Just an Idea )

          Show
          Nadav Kavalerchik added a comment - Here is a link to a Hack I made years ago on Moodle 1.9.2 (because of many Teachers heavy request to simplify the UI. which was also mentioned by Josef, on the above previous comment) on Moodle 2+ I have not had the time to hack something similar and was hoping this Issue will gain momentum and get implemented in core. See it in action: http://www.youtube.com/watch?v=I9I-usZ2lsA And some snippets of code I dug up (hope I did not leave any important piece out) The actual hide/show javascript code: $mform->addElement(' static ', 'hidingjscode', '', '<script> function shownextchildclass(id) { if ( document.getElementById(id).parentNode.nextSibling.nextSibling.nextSibling.attributes[0].nodeValue == \'fcontainer clearfix\') { document.getElementById(id).parentNode.nextSibling.nextSibling.nextSibling.attributes[0].nodeValue = \'fcontainer clearfix hide\'; } else { document.getElementById(id).parentNode.nextSibling.nextSibling.nextSibling.attributes[0].nodeValue = \'fcontainer clearfix\'; } //alert(document.getElementById(\'id_toggel1\').parentNode.nextSibling.nextSibling.nextSibling.attributes[0].nodeValue); } function hideheaders() { document.getElementById(\'id_enrolmentsheader\').parentNode.parentNode.childNodes[4].attributes[0].nodeValue = \'fcontainer clearfix hide\'; document.getElementById(\'id_expirynotifyhdrheader\').parentNode.parentNode.childNodes[4].attributes[0].nodeValue = \'fcontainer clearfix hide\'; document.getElementById(\'id_rolerenamingheader\').parentNode.parentNode.childNodes[4].attributes[0].nodeValue = \'fcontainer clearfix hide\'; } window.addEventListener(\'load\',hideheaders, false ); </script> <style> .hide { display:none; } </style> '); And one of the onClick javascript function + image that invoke the hide/show //$mform->addElement('header','enrolhdr', get_string('enrolments')); $mform->addElement('header', 'enrolhdr', get_string('enrolments'). ' <img src= "'.$CFG->wwwroot.'/mod/tab/pix/arrow-down- double .png" id= "id_enrolmentsheader" onclick= "shownextchildclass(\'id_enrolmentsheader\')" >'); Obviously, the above code is not a usage suggestion of any kind. just to show that the need is very old and that I had to work=a=round the current Form's library to hack my way so the teachers could use the system more easily. BTW, showing the teachers the option to "show in advanced" was not helpful enough. Most (80%) teachers (Basic Content Creators) would like to be able to change two...three important items on any Form and submit it. They are intimidated by even seeing all the options. (If we had more teachers in the tracker, they could verify this. I am sure) I am even suggesting the add two Tabs on the top of each Form, with "Basic" and "Advanced" titles. which will default to "Basic" in which it will display to the teachers two...three most important input fields (items). And if they are more experienced and advanced Teachers (Content Creators) they can switch view to the "Advanced" tab and fill out more settings. ( Just an Idea )
          Hide
          Colin Chambers added a comment -

          Great work guys. We've been working on improving the question editing forms. We've implemented collapsible forms too. Your version seems well made and designed for site wide implementation. So we're hoping to use your work for the collapsible element. Saving use work .

          One small but useful suggestion is to add a * or other marker to the title of a section that has a required field. It just makes it easier for people to identify which sections to check if the form won't submit. I think the validation errors in moodle appear next to the field. It's just possible the user hid the section for some reason and won't then see either the field or the error. They'll just think the form or moodle is broken in some way.

          This way they know exactly which sections to check.

          I think it was relatively easy to implement. HTH.

          Show
          Colin Chambers added a comment - Great work guys. We've been working on improving the question editing forms . We've implemented collapsible forms too. Your version seems well made and designed for site wide implementation. So we're hoping to use your work for the collapsible element. Saving use work . One small but useful suggestion is to add a * or other marker to the title of a section that has a required field. It just makes it easier for people to identify which sections to check if the form won't submit. I think the validation errors in moodle appear next to the field. It's just possible the user hid the section for some reason and won't then see either the field or the error. They'll just think the form or moodle is broken in some way. This way they know exactly which sections to check. I think it was relatively easy to implement. HTH.
          Hide
          Damyon Wiese added a comment -

          Thanks Colin, good suggestion about the required fields "*". Lets call that one point 10.

          Re: Removing "Show advanced"
          The problem with show advanced - is you have to click it at least once to see what you're missing. So it hides settings under a generic name that does not describe what it does. You could have the same thing with these shortforms by adding a section called "Advanced settings" (but I wouldn't recommend it).

          I think when designing the layout of the form, the most commonly used, simple settings should be in the first or second section and the more advanced settings further down. (I did this for assignment in the screenshots on the linked issue).

          Point 11.
          Another suggestion (for bonus points) is that on a form with a reasonable number of sections (see the linked assignment issue for screenshots). Even when closed, the sections headings take up quite a lot of vertical space in the form. This pushes the important "Save", "Cancel" etc buttons off the bottom of the page on most screens. My suggestion here is that if multiple sections in a row are collapsed, the second, third etc should be listed horizontally on the same line as the first (allowing for wrapping).

          Show
          Damyon Wiese added a comment - Thanks Colin, good suggestion about the required fields "*". Lets call that one point 10. Re: Removing "Show advanced" The problem with show advanced - is you have to click it at least once to see what you're missing. So it hides settings under a generic name that does not describe what it does. You could have the same thing with these shortforms by adding a section called "Advanced settings" (but I wouldn't recommend it). I think when designing the layout of the form, the most commonly used, simple settings should be in the first or second section and the more advanced settings further down. (I did this for assignment in the screenshots on the linked issue). Point 11. Another suggestion (for bonus points) is that on a form with a reasonable number of sections (see the linked assignment issue for screenshots). Even when closed, the sections headings take up quite a lot of vertical space in the form. This pushes the important "Save", "Cancel" etc buttons off the bottom of the page on most screens. My suggestion here is that if multiple sections in a row are collapsed, the second, third etc should be listed horizontally on the same line as the first (allowing for wrapping).
          Hide
          Damyon Wiese added a comment -

          I'll add some patches to cover the issues I raised.

          Show
          Damyon Wiese added a comment - I'll add some patches to cover the issues I raised.
          Hide
          Tim Hunt added a comment -

          Re 11. are you sure you can't solve the problem by appropriate CSS to remove unnecessary vertical space? Switching to side-by-side could end up being horrible and counter-intuitive. But, perhaps it is worth taking the time to code up various options, and trying them out (ideally with users) to make sure that the one we finally go for is the best.

          Re "the most commonly used, simple settings should be in the first or second section and the more advanced settings further down". Really? No shit! I am sorry, but your suggestion just comes across as patronising. Before saying that, did you look at the quiz settings form? We do exactly that already, but in addition, in several of the sections of "advance settings lower down" we also hide some of the options behind show advanced, and I think that is a usability win.

          And it is not just usability. At the OU, we basically never want people to use most of the "Extra restrictions on attempts" options. It is great for us that we can effectively hide these by making them advanced. That makes things less confusing for our staff, and reduces the number of instances of wrong settings that our support staff have to deal with.

          I am not saying that we must keep Show advanced, but I am trying to make hte case that there are some usability benefits that can be achieved with Show advanced which cannot be achieved with short-forms yet, and that perhaps we should try to think of alternative (perhaps even better) ways to get the same usability benefits before we remove strip out show advanced.

          Here is an idea. Perhaps, for sections containing advanced fields, the hide / show for the section has three states hide / show common / show all?

          Show
          Tim Hunt added a comment - Re 11. are you sure you can't solve the problem by appropriate CSS to remove unnecessary vertical space? Switching to side-by-side could end up being horrible and counter-intuitive. But, perhaps it is worth taking the time to code up various options, and trying them out (ideally with users) to make sure that the one we finally go for is the best. Re "the most commonly used, simple settings should be in the first or second section and the more advanced settings further down". Really? No shit! I am sorry, but your suggestion just comes across as patronising. Before saying that, did you look at the quiz settings form? We do exactly that already, but in addition, in several of the sections of "advance settings lower down" we also hide some of the options behind show advanced, and I think that is a usability win. And it is not just usability. At the OU, we basically never want people to use most of the "Extra restrictions on attempts" options. It is great for us that we can effectively hide these by making them advanced. That makes things less confusing for our staff, and reduces the number of instances of wrong settings that our support staff have to deal with. I am not saying that we must keep Show advanced, but I am trying to make hte case that there are some usability benefits that can be achieved with Show advanced which cannot be achieved with short-forms yet, and that perhaps we should try to think of alternative (perhaps even better) ways to get the same usability benefits before we remove strip out show advanced. Here is an idea. Perhaps, for sections containing advanced fields, the hide / show for the section has three states hide / show common / show all?
          Hide
          Philip Butcher added a comment -

          Re. the proposal to remove Show Advanced.

          > Most (80%) teachers (Basic Content Creators) would like to be able to change two...three important items on any Form and submit it. They are intimidated by even seeing all the options. (If we had more teachers in the tracker, they could verify this. I am sure).

          Let me come in here as a voice for the teachers creating Quizzes on 125 modules at the OU (clearly I'm with Tim). We are talking about a lot of quizzes. We are also talking about a lot of staff who have no interest in all the possible settings for the quiz. We preset as much as we can and we hide as much as we can with Show Advanced. The comment above is spot on. For most of our quizzes the authors have to set 4 items.

          I am not at all happy about the idea of losing 'Show advanced'.

          Show
          Philip Butcher added a comment - Re. the proposal to remove Show Advanced. > Most (80%) teachers (Basic Content Creators) would like to be able to change two...three important items on any Form and submit it. They are intimidated by even seeing all the options. (If we had more teachers in the tracker, they could verify this. I am sure). Let me come in here as a voice for the teachers creating Quizzes on 125 modules at the OU (clearly I'm with Tim). We are talking about a lot of quizzes. We are also talking about a lot of staff who have no interest in all the possible settings for the quiz. We preset as much as we can and we hide as much as we can with Show Advanced. The comment above is spot on. For most of our quizzes the authors have to set 4 items. I am not at all happy about the idea of losing 'Show advanced'.
          Hide
          Ruslan Kabalin added a comment -

          Damyon: regarding point 11, do you want to arrange collapsed sections at the same line? They will not look good if I understand correctly, also what will happen if middle one will be expanded, they will need to be rearranged?
          When do you plan to add patches, just asking in order not to do the same? I plan to focus on completing this issue this week.

          Tim: Regarding advanced settings, we can obviously leave it where it is now, but I personally think it will be confusing, A separate section of advanced things is not an option. By the way, we have short forms enabled in Lancaster University for over a year, no one complained about short forms usability in any way yet, only positive feedback was received.

          Do you guys agree with proposed behaviour below that was implemented?

          • always displaying the 'General' section and closing all others by default;
          • always displaying a section with a required element;
          • opening any section which contains validation errors;
          • opening any section which was previously open on previous submit (e.g. when adding new choices);
          • displaying elements which are coded as expanded by default (e.g. $mform->setExpanded('foo');

          Just asking because we are still hitting from time to time forms where useful sections are collapsed, so now I think that having everything expanded by default might be a good option (collapse only those sections that were marked as collapsed $mform->setCollapsed('foo')). This is basically Sam's suggestion. What do you think?

          Show
          Ruslan Kabalin added a comment - Damyon: regarding point 11, do you want to arrange collapsed sections at the same line? They will not look good if I understand correctly, also what will happen if middle one will be expanded, they will need to be rearranged? When do you plan to add patches, just asking in order not to do the same? I plan to focus on completing this issue this week. Tim: Regarding advanced settings, we can obviously leave it where it is now, but I personally think it will be confusing, A separate section of advanced things is not an option. By the way, we have short forms enabled in Lancaster University for over a year, no one complained about short forms usability in any way yet, only positive feedback was received. Do you guys agree with proposed behaviour below that was implemented? always displaying the 'General' section and closing all others by default; always displaying a section with a required element; opening any section which contains validation errors; opening any section which was previously open on previous submit (e.g. when adding new choices); displaying elements which are coded as expanded by default (e.g. $mform->setExpanded('foo'); Just asking because we are still hitting from time to time forms where useful sections are collapsed, so now I think that having everything expanded by default might be a good option (collapse only those sections that were marked as collapsed $mform->setCollapsed('foo')). This is basically Sam's suggestion . What do you think?
          Hide
          Andrew Nicols added a comment -

          I'm actually going to disagree with you there Ruslan - I think that the default collapsed for most forms.

          There are relatively few forms which you want to have default all open, but there are often individual sections you want to be open by default.
          For the most part these are covered by the open-if-required rule, and adding Damyon's point 3 (disabling auto-collapse on single-section forms) seems like a really good idea to complement this. I think that it would actually cover most of the cases currently missed.

          On the 'Show advanced' discussion, I can see the benefits of keeping some form of advanced settings toggle, but I think that it needs to change in some way. From my perspective, most Moodle forms wouldn't want the advanced toggle if short forms was implemented - it seems that it's mostly just the quiz (and possibly assignments) forms where the settings in Advanced truely are advanced.
          I suspect that this is largely an issue caused by the type of use of the advanced toggle in legacy code - i.e. the settings weren't so much 'Advanced', as less interesting to most people.

          Show
          Andrew Nicols added a comment - I'm actually going to disagree with you there Ruslan - I think that the default collapsed for most forms. There are relatively few forms which you want to have default all open, but there are often individual sections you want to be open by default. For the most part these are covered by the open-if-required rule, and adding Damyon's point 3 (disabling auto-collapse on single-section forms) seems like a really good idea to complement this. I think that it would actually cover most of the cases currently missed. On the 'Show advanced' discussion, I can see the benefits of keeping some form of advanced settings toggle, but I think that it needs to change in some way. From my perspective, most Moodle forms wouldn't want the advanced toggle if short forms was implemented - it seems that it's mostly just the quiz (and possibly assignments) forms where the settings in Advanced truely are advanced. I suspect that this is largely an issue caused by the type of use of the advanced toggle in legacy code - i.e. the settings weren't so much 'Advanced', as less interesting to most people.
          Hide
          Tim Hunt added a comment -

          I think Andrew's comment about Show advanced is about right. After short-forms are implemented, we should review all use of setAdvanced, and remove MOST of them. But it would be wrong to remove ALL of them.

          Show
          Tim Hunt added a comment - I think Andrew's comment about Show advanced is about right. After short-forms are implemented, we should review all use of setAdvanced, and remove MOST of them. But it would be wrong to remove ALL of them.
          Hide
          Ruslan Kabalin added a comment -

          Tim: so, what would you suggest as solution, keep the support or advanced button as it is? May be we could change the design slightly so that all advanced stuff will appear at the bottom of the section after pressing the button, or even turn the button to the link at the bottom of the section, clicking on which will do further expansion of the same section showing extra stuff below?

          Show
          Ruslan Kabalin added a comment - Tim: so, what would you suggest as solution, keep the support or advanced button as it is? May be we could change the design slightly so that all advanced stuff will appear at the bottom of the section after pressing the button, or even turn the button to the link at the bottom of the section, clicking on which will do further expansion of the same section showing extra stuff below?
          Hide
          Andrew Nicols added a comment -

          Tim: I agree, we do need to review the existing uses of setAdvanced (MDL-30806 should probably cover this too), but perhaps we also need to consider the display where JavaScript is disabled. I don't know what effect that would have on removing setAdvanced in a majority of cases.

          That said... perhaps it's time that we stopped supporting everything when JS is disabled and display the full, un-condensed form. All functionality is still available after all.

          Do you have any stats which suggest the actual level of non-JS activity within the OU for example?

          Show
          Andrew Nicols added a comment - Tim: I agree, we do need to review the existing uses of setAdvanced ( MDL-30806 should probably cover this too), but perhaps we also need to consider the display where JavaScript is disabled. I don't know what effect that would have on removing setAdvanced in a majority of cases. That said... perhaps it's time that we stopped supporting everything when JS is disabled and display the full, un-condensed form. All functionality is still available after all. Do you have any stats which suggest the actual level of non-JS activity within the OU for example?
          Hide
          Nadav Kavalerchik added a comment -

          I wonder if we could implement a "Show Advanced" feature that control the TinyMCE (or any editor in the "htmleditor" form element) Which will enable us to display the initial state of the "htmleditor" in simplified view - one line of important icons + a "Show more" icon, Or an Advanced view - All the available icons, depending on element's needs.

          It could further simplify the UI.

          Show
          Nadav Kavalerchik added a comment - I wonder if we could implement a "Show Advanced" feature that control the TinyMCE (or any editor in the "htmleditor" form element) Which will enable us to display the initial state of the "htmleditor" in simplified view - one line of important icons + a "Show more" icon, Or an Advanced view - All the available icons, depending on element's needs. It could further simplify the UI.
          Hide
          Tim Hunt added a comment -

          What I suggest is the last two paragraphs of this comment: https://tracker.moodle.org/browse/MDL-30637?focusedCommentId=196377&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-196377

          @Andrew, reviewing setAdvanced calls could also be a separate subtask. I think JS disabled is now very rare, but I have no data.

          @Nadav, see https://tracker.moodle.org/browse/MDL-32750. We are tackling that.

          Show
          Tim Hunt added a comment - What I suggest is the last two paragraphs of this comment: https://tracker.moodle.org/browse/MDL-30637?focusedCommentId=196377&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-196377 @Andrew, reviewing setAdvanced calls could also be a separate subtask. I think JS disabled is now very rare, but I have no data. @Nadav, see https://tracker.moodle.org/browse/MDL-32750 . We are tackling that.
          Hide
          Ruslan Kabalin added a comment -

          Perhaps, for sections containing advanced fields, the hide / show for the section has three states hide / show common / show all?

          I like this idea.

          Show
          Ruslan Kabalin added a comment - Perhaps, for sections containing advanced fields, the hide / show for the section has three states hide / show common / show all? I like this idea.
          Hide
          Nadav Kavalerchik added a comment - - edited

          @Tim, thank you for the MDL-32750 link. I was not aware of it. (just now tested it)
          Although it is a great move in the right direction, I was hoping to see that we leave a single toolbar with important icons for an initial basic view of the editor and not remove them completely.
          But still, MDL-32750 is much much better than what we have now. great (beautiful) work!

          Show
          Nadav Kavalerchik added a comment - - edited @Tim, thank you for the MDL-32750 link. I was not aware of it. (just now tested it) Although it is a great move in the right direction, I was hoping to see that we leave a single toolbar with important icons for an initial basic view of the editor and not remove them completely. But still, MDL-32750 is much much better than what we have now. great (beautiful) work!
          Hide
          David Mudrak added a comment -

          simple settings should be in the first or second section and the more advanced settings further down

          I do not think such an approach might be applicable all the time. Often, sections are used to group relevant settings together. Of those, some are just naturally "common" or "basic" and some are "advanced". Splitting them into separate sections may lead to loosing the context of the setting and thence worse usability.

          Perhaps, for sections containing advanced fields, the hide / show for the section has three states hide / show common / show all?

          +1 I like this idea.

          Show
          David Mudrak added a comment - simple settings should be in the first or second section and the more advanced settings further down I do not think such an approach might be applicable all the time. Often, sections are used to group relevant settings together. Of those, some are just naturally "common" or "basic" and some are "advanced". Splitting them into separate sections may lead to loosing the context of the setting and thence worse usability. Perhaps, for sections containing advanced fields, the hide / show for the section has three states hide / show common / show all? +1 I like this idea.
          Hide
          Damyon Wiese added a comment -

          Ruslan: I was going to implement some of these suggestions (the agreed on ones) as patches, but I'll leave it if you still working on this (Was just looking to keep this moving).

          Tim: - my suggestion about moving the most common settings to the first or second section relates specifically to this feature. I wasn't talking about the quiz or any form in particular - just whether we should aim for the goal of having the most used settings open by default and the rest closed. In the linked assignment issue I have tried to do this (but it is hard to say what the most used settings are). As David pointed out - some forms are grouped by features and a feature can have a mix of common and uncommon settings. The hide / show common / show all suggestion sounds good - as long as there is a way to show all for the entire form with one click.

          Re: Point 11 - I don't know if that would work nicely or not - its just a suggestion. But it would be nice to reduce the vertical scrolling in most forms (especially if we can get to the submit button without scrolling by default). I think we shouldn't look at this now - but it might be a nice improvement later (might).

          Re: "everything expanded by default" -1 for this from me.

          Show
          Damyon Wiese added a comment - Ruslan: I was going to implement some of these suggestions (the agreed on ones) as patches, but I'll leave it if you still working on this (Was just looking to keep this moving). Tim: - my suggestion about moving the most common settings to the first or second section relates specifically to this feature. I wasn't talking about the quiz or any form in particular - just whether we should aim for the goal of having the most used settings open by default and the rest closed. In the linked assignment issue I have tried to do this (but it is hard to say what the most used settings are). As David pointed out - some forms are grouped by features and a feature can have a mix of common and uncommon settings. The hide / show common / show all suggestion sounds good - as long as there is a way to show all for the entire form with one click. Re: Point 11 - I don't know if that would work nicely or not - its just a suggestion. But it would be nice to reduce the vertical scrolling in most forms (especially if we can get to the submit button without scrolling by default). I think we shouldn't look at this now - but it might be a nice improvement later (might). Re: "everything expanded by default" -1 for this from me.
          Hide
          Damyon Wiese added a comment -

          Martin had a good suggestion "show advanced" option.

          Keep the api call to set a form field as advanced - but automatically create a separate section for advanced settings with "<sectionname> (Advanced)" as the name. So you can show the basic/advanced settings separately without extra buttons.

          Show
          Damyon Wiese added a comment - Martin had a good suggestion "show advanced" option. Keep the api call to set a form field as advanced - but automatically create a separate section for advanced settings with "<sectionname> (Advanced)" as the name. So you can show the basic/advanced settings separately without extra buttons.
          Hide
          Nadav Kavalerchik added a comment -

          How About putting all in TABs: "Basic", "Common", "Advanced" ?
          It will have a short view on the page. Most important items will appear on "Basic" and all other items, with default values, will appear on "Common" and "Advanced" Tabs. And we could add "Buttons" on the bottom of each Tab to help the user navigate to the other Tabs when she/he reaches the end of the form on that Tab

          Show
          Nadav Kavalerchik added a comment - How About putting all in TABs: "Basic", "Common", "Advanced" ? It will have a short view on the page. Most important items will appear on "Basic" and all other items, with default values, will appear on "Common" and "Advanced" Tabs. And we could add "Buttons" on the bottom of each Tab to help the user navigate to the other Tabs when she/he reaches the end of the form on that Tab
          Hide
          Martin Dougiamas added a comment -

          Don't you think this is going to make the forms interface quite complex-looking?

          Show
          Martin Dougiamas added a comment - Don't you think this is going to make the forms interface quite complex-looking?
          Hide
          Damyon Wiese added a comment -

          The problem with that Nadav (IMO) is that if I'm looking at the assignment and I want to enable "Students submit in groups" - which section is that in ? I have to hunt for it.

          Show
          Damyon Wiese added a comment - The problem with that Nadav (IMO) is that if I'm looking at the assignment and I want to enable "Students submit in groups" - which section is that in ? I have to hunt for it.
          Hide
          Damyon Wiese added a comment -

          Some progress on this:

          >> If we completely re-did forms lib, we could re-do that ability for admins to
          >> customise forms in a way that applies to any form. That would be cool, but is not
          >> going to happen any time soon.
          >
          > Actually me and Damyon were talking about how he wants to do that in assignment
          > too, and we have the same thing in the course settings too and i'm sure elsewhere. > So we should consider if there is a fast/cheap way of doing this!

          Here: https://tracker.moodle.org/browse/MDL-37459

          Would appreciate comments!

          Show
          Damyon Wiese added a comment - Some progress on this: >> If we completely re-did forms lib, we could re-do that ability for admins to >> customise forms in a way that applies to any form. That would be cool, but is not >> going to happen any time soon. > > Actually me and Damyon were talking about how he wants to do that in assignment > too, and we have the same thing in the course settings too and i'm sure elsewhere. > So we should consider if there is a fast/cheap way of doing this! Here: https://tracker.moodle.org/browse/MDL-37459 Would appreciate comments!
          Hide
          Alan Arnold added a comment - - edited

          This is a marvellous thread, with exciting possibilities. I'm struck by the notion that "Advanced" or "Basic" descriptors are in the eye of the beholder (and a bit presumptuous) and I wonder whether we could benefit from design ideas that have been around a long time in other areas.

          Imagine you wanted to set up your favourite printer from a Wintel machine to be double-sided, colour, two-pages in one, stapled. Most of the time, you're happy with just greyscale, double-sided. As a Mac user, I can never remember which screen in the printer setup to go to to find these (progessively disclosed or not) and I begrudgingly appreciate the ability to save those configuration sets with names that mean something to me, and reuse them with a single click/pulldown.

          I think many of my teaching colleagues might like the same convenience to set up assignment or quiz settings once for a few of their common use-cases, and re-use them many times. It would also be nice if their admins could set up a few reusable sets of preferences that might be starting points for them, to meet institutional needs

          Just a thought.

          Show
          Alan Arnold added a comment - - edited This is a marvellous thread, with exciting possibilities. I'm struck by the notion that "Advanced" or "Basic" descriptors are in the eye of the beholder (and a bit presumptuous) and I wonder whether we could benefit from design ideas that have been around a long time in other areas. Imagine you wanted to set up your favourite printer from a Wintel machine to be double-sided, colour, two-pages in one, stapled. Most of the time, you're happy with just greyscale, double-sided. As a Mac user, I can never remember which screen in the printer setup to go to to find these (progessively disclosed or not) and I begrudgingly appreciate the ability to save those configuration sets with names that mean something to me, and reuse them with a single click/pulldown. I think many of my teaching colleagues might like the same convenience to set up assignment or quiz settings once for a few of their common use-cases, and re-use them many times. It would also be nice if their admins could set up a few reusable sets of preferences that might be starting points for them, to meet institutional needs Just a thought.
          Hide
          Ruslan Kabalin added a comment -

          Rebased to the current master and squashed some minor bits: https://git.luns.net.uk/moodle.git/shortlog/refs/heads/luns-shortforms-rebased

          Show
          Ruslan Kabalin added a comment - Rebased to the current master and squashed some minor bits: https://git.luns.net.uk/moodle.git/shortlog/refs/heads/luns-shortforms-rebased
          Hide
          Tim Hunt added a comment -

          I like the look of Show more/less... in the recently added screen-grabs.

          Show
          Tim Hunt added a comment - I like the look of Show more/less... in the recently added screen-grabs.
          Hide
          Ruslan Kabalin added a comment - - edited

          Right, I had a closer look at the advanced elements stuff and come to the idea that three-states show/show all/hide control which Tim suggested and which I liked initially, is not that good. The primary problem is action feedback and usability. There are several use case when this behaviour will be confusing IMHO:

          1. Imagine the case when someone just want to open a section to look up what it contains, user clicks on the link, section gets expanded, user sees that this is not the one he needs, he clicks again but instead of closing it, he sees even more options.
          2. User made some changes and wants to close the section, he clicks on the link and sees more options instead.
          3. User clicks twice on the section link, reached advanced options display, but now wants to switch back to normal, he needs to click twice on the link again so that first click will close the section and the second one will open it again in normal mode.

          Another question here, how to feedback user in which state he is now and hint that advanced options are available? Different triangle icon colour and style? A sign that advanced options are available, click on the expand link again?

          I have designed some mock-ups to implement "show advanced" the easier and better way I think. Can you please have a look and let me know if you like any them.

          Unlike the current "show advanced" button behaviour, I suggest that only the current section advanced options will be displayed and hidden depending on the state.

          I am not in favour if adding the button that will make all advanced options on the whole page visible to be honest, if this is absolutely required may be this can be a user setting?

          Show
          Ruslan Kabalin added a comment - - edited Right, I had a closer look at the advanced elements stuff and come to the idea that three-states show/show all/hide control which Tim suggested and which I liked initially, is not that good. The primary problem is action feedback and usability. There are several use case when this behaviour will be confusing IMHO: Imagine the case when someone just want to open a section to look up what it contains, user clicks on the link, section gets expanded, user sees that this is not the one he needs, he clicks again but instead of closing it, he sees even more options. User made some changes and wants to close the section, he clicks on the link and sees more options instead. User clicks twice on the section link, reached advanced options display, but now wants to switch back to normal, he needs to click twice on the link again so that first click will close the section and the second one will open it again in normal mode. Another question here, how to feedback user in which state he is now and hint that advanced options are available? Different triangle icon colour and style? A sign that advanced options are available, click on the expand link again? I have designed some mock-ups to implement "show advanced" the easier and better way I think. Can you please have a look and let me know if you like any them. Unlike the current "show advanced" button behaviour, I suggest that only the current section advanced options will be displayed and hidden depending on the state. I am not in favour if adding the button that will make all advanced options on the whole page visible to be honest, if this is absolutely required may be this can be a user setting?
          Hide
          Ruslan Kabalin added a comment -

          I like the look of Show more/less...

          Yeah, that the one I like more as well, Andrew Nicols' idea in fact.

          Show
          Ruslan Kabalin added a comment - I like the look of Show more/less... Yeah, that the one I like more as well, Andrew Nicols' idea in fact.
          Hide
          Nadav Kavalerchik added a comment -

          I like the look of Show more/less... in the recently added screen-grabs.

          I like it too.

          Show
          Nadav Kavalerchik added a comment - I like the look of Show more/less... in the recently added screen-grabs. I like it too.
          Hide
          Damyon Wiese added a comment -

          Yes - I like show more/show less too.

          Show
          Damyon Wiese added a comment - Yes - I like show more/show less too.
          Hide
          Dan Poltawski added a comment -

          I like the look of Show more/less...

          +1 here too

          Show
          Dan Poltawski added a comment - I like the look of Show more/less... +1 here too
          Hide
          Martin Dougiamas added a comment -

          +1 from me too, I think we have a winner. Nice job guys.

          Show
          Martin Dougiamas added a comment - +1 from me too, I think we have a winner. Nice job guys.
          Hide
          Ruslan Kabalin added a comment -

          Thanks for the feedback guys, I start implementing Show more/less design.

          Show
          Ruslan Kabalin added a comment - Thanks for the feedback guys, I start implementing Show more/less design.
          Hide
          Dan Poltawski added a comment -

          Might it be worth starting a new discussion about this in the future features forum (or moving/reignighting[1] the old one). I think the old one was split on this an the activity chooser so a new one might be beneficial.

          [1] Eloy really does influence my language these days!

          Show
          Dan Poltawski added a comment - Might it be worth starting a new discussion about this in the future features forum (or moving/reignighting [1] the old one). I think the old one was split on this an the activity chooser so a new one might be beneficial. [1] Eloy really does influence my language these days!
          Hide
          Aparup Banerjee added a comment -

          a post-race +1 to show more/less. its really much less imposing.

          Show
          Aparup Banerjee added a comment - a post-race +1 to show more/less. its really much less imposing.
          Hide
          Ruslan Kabalin added a comment -

          Hello, I have updated the branch having added show more/less functionality. Below is the summary of functional changes:

          1. The Advanced button is replaced by the Show Less/More link as has been suggested and agreed in discussion above.
          2. The link which controls advanced elements displaying will only work within the section it is displayed at (opposed to the past case when clicking on Advanced button displayed all advanced elements throughout the form).
          3. The Advanced state of sections is preserved between form submissions.
          4. If Javascript is off, all advanced elements will be displayed on the page by default (but will be marked with green asterisks as it is now), no show/hide controls will exists on the page.
          5. If some of the advanced elements will contain error upon submission, the section with an element with an error will be expanded and advanced elements displayed to highlight the error for user.

          Any comments are welcome.

          Show
          Ruslan Kabalin added a comment - Hello, I have updated the branch having added show more/less functionality. Below is the summary of functional changes: The Advanced button is replaced by the Show Less/More link as has been suggested and agreed in discussion above. The link which controls advanced elements displaying will only work within the section it is displayed at (opposed to the past case when clicking on Advanced button displayed all advanced elements throughout the form). The Advanced state of sections is preserved between form submissions. If Javascript is off, all advanced elements will be displayed on the page by default (but will be marked with green asterisks as it is now), no show/hide controls will exists on the page. If some of the advanced elements will contain error upon submission, the section with an element with an error will be expanded and advanced elements displayed to highlight the error for user. Any comments are welcome.
          Hide
          Tim Hunt added a comment -

          Generally this is looking good. I would really like to see this integrated soon.

          On a fairly quick look through the patch, I noticed the following points.

          1. Have we already discussed whether we should have an admin setting for this? I think the new hehavioru is just better, so new admin_setting_configcheckbox('enableshortmenus', ...) is unnecessary.

          2. This is probably irrelevant, but the language string for that setting is confusing: "This enables javascript shortened menus", we are dealing with forms and collapsing sections, not menus, and users don't care that this is implemented with JavaScript.

          3. Both your new JavaScript modules require 'moodle-enrol-notification'. That is a crazy dependency! Is it really necessary? If so, can we change it to make it unnecessary.

          4. Is this really the way we want to do this?

          // ensure that general fieldset is non-collapsable
          $generalfieldsets = array('general', 'generalheader', 'configheader');
          

          Surely it is simpler to say "first section of the form" rather than "section name in this hard-coded list".

          5. From the patch, I cannot immediately deduce the logic you used for which forms have $mform->setDisableShortforms();

          Show
          Tim Hunt added a comment - Generally this is looking good. I would really like to see this integrated soon. On a fairly quick look through the patch, I noticed the following points. 1. Have we already discussed whether we should have an admin setting for this? I think the new hehavioru is just better, so new admin_setting_configcheckbox('enableshortmenus', ...) is unnecessary. 2. This is probably irrelevant, but the language string for that setting is confusing: "This enables javascript shortened menus", we are dealing with forms and collapsing sections, not menus, and users don't care that this is implemented with JavaScript. 3. Both your new JavaScript modules require 'moodle-enrol-notification'. That is a crazy dependency! Is it really necessary? If so, can we change it to make it unnecessary. 4. Is this really the way we want to do this? // ensure that general fieldset is non-collapsable $generalfieldsets = array('general', 'generalheader', 'configheader'); Surely it is simpler to say "first section of the form" rather than "section name in this hard-coded list". 5. From the patch, I cannot immediately deduce the logic you used for which forms have $mform->setDisableShortforms();
          Hide
          Ruslan Kabalin added a comment - - edited

          Thanks Tim for the comments.

          1. Have we already discussed whether we should have an admin setting for this? I think the new hehavioru is just better, so new admin_setting_configcheckbox('enableshortmenus', ...) is unnecessary.

          True, I have removed this config parameter (see the last commit on the branch).

          2. This is probably irrelevant, but the language string for that setting is confusing: "This enables javascript shortened menus", we are dealing with forms and collapsing sections, not menus, and users don't care that this is implemented with JavaScript.

          Removed as well.

          3. Both your new JavaScript modules require 'moodle-enrol-notification'. That is a crazy dependency! Is it really necessary? If so, can we change it to make it unnecessary.

          Thanks, moodle-enrol-notification has been depricated recently, changed as required.

          4. Is this really the way we want to do this?

          // ensure that general fieldset is non-collapsable

          $generalfieldsets = array('general', 'generalheader', 'configheader');

          Surely it is simpler to say "first section of the form" rather than "section name in this hard-coded list".

          What if "general" is not at the top? Or we are dealing with the page that does not have "general" section at all?

          5. From the patch, I cannot immediately deduce the logic you used for which forms have $mform->setDisableShortforms();

          Backup/importing/exporting, basically the cases when shortforms look ugly and complicates the choice. In all these cases the top section is not general - you reckon we should expand the top one by default to avoid using disable in such cases?

          Show
          Ruslan Kabalin added a comment - - edited Thanks Tim for the comments. 1. Have we already discussed whether we should have an admin setting for this? I think the new hehavioru is just better, so new admin_setting_configcheckbox('enableshortmenus', ...) is unnecessary. True, I have removed this config parameter (see the last commit on the branch). 2. This is probably irrelevant, but the language string for that setting is confusing: "This enables javascript shortened menus", we are dealing with forms and collapsing sections, not menus, and users don't care that this is implemented with JavaScript. Removed as well. 3. Both your new JavaScript modules require 'moodle-enrol-notification'. That is a crazy dependency! Is it really necessary? If so, can we change it to make it unnecessary. Thanks, moodle-enrol-notification has been depricated recently, changed as required. 4. Is this really the way we want to do this? // ensure that general fieldset is non-collapsable $generalfieldsets = array('general', 'generalheader', 'configheader'); Surely it is simpler to say "first section of the form" rather than "section name in this hard-coded list". What if "general" is not at the top? Or we are dealing with the page that does not have "general" section at all? 5. From the patch, I cannot immediately deduce the logic you used for which forms have $mform->setDisableShortforms(); Backup/importing/exporting, basically the cases when shortforms look ugly and complicates the choice. In all these cases the top section is not general - you reckon we should expand the top one by default to avoid using disable in such cases?
          Hide
          Tim Hunt added a comment -

          4. & 5. grep for "->addElement('header'" seems to be the good way to see the section headings of all forms. (~400 matches in the code). From this I deduce:

          • Many forms have really bad section names. We should not rely on them at all.

          More seriously

          • There are lots of forms with just one section. For these, we should automatically disable shortforms.
          • In other cases, the logic "form should be collapsible, all sections but the first collapsed by default" is the right default.

          Acutally, it might be worth reviewing all forms with 2 sections, to see if we should make the rule "All forms with one or two sections should have collapsing disabled by default."

          Anyway, those two rules should maximise the number of forms that just automatically work.

          (As it happens, from what I can see, whenever there is a general section to a form, it does comes first.)

          Show
          Tim Hunt added a comment - 4. & 5. grep for "->addElement('header'" seems to be the good way to see the section headings of all forms. (~400 matches in the code). From this I deduce: Many forms have really bad section names. We should not rely on them at all. More seriously There are lots of forms with just one section. For these, we should automatically disable shortforms. In other cases, the logic "form should be collapsible, all sections but the first collapsed by default" is the right default. Acutally, it might be worth reviewing all forms with 2 sections, to see if we should make the rule "All forms with one or two sections should have collapsing disabled by default." Anyway, those two rules should maximise the number of forms that just automatically work. (As it happens, from what I can see, whenever there is a general section to a form, it does comes first.)
          Hide
          Andrew Nicols added a comment -

          We should be able to entirely drop the dependency on moodle-core-notification.

          Show
          Andrew Nicols added a comment - We should be able to entirely drop the dependency on moodle-core-notification.
          Hide
          Tim Hunt added a comment -

          Having got back from lunch and looked through the search results more closely, I think the good heuristic for default behaviour is:

          • 1- or 2- section forms not collapsible.
          • 3+ section form forms collapsible, with the first section expanded by default and the rest collapsed.

          Of course, any particular form can then override this, but I don't think many will need to.

          Show
          Tim Hunt added a comment - Having got back from lunch and looked through the search results more closely, I think the good heuristic for default behaviour is: 1- or 2- section forms not collapsible. 3+ section form forms collapsible, with the first section expanded by default and the rest collapsed. Of course, any particular form can then override this, but I don't think many will need to.
          Hide
          Dan Poltawski added a comment -

          I've not reviewed the code again, would just like to say thanks Tim for reviewing it. Recently I have been really encouraged by how much feedback we're starting to see on new things going to core. Reminds me of the old days

          Show
          Dan Poltawski added a comment - I've not reviewed the code again, would just like to say thanks Tim for reviewing it. Recently I have been really encouraged by how much feedback we're starting to see on new things going to core. Reminds me of the old days
          Hide
          Dan Poltawski added a comment -

          Just re-read the comments, I think Tim's initial comments were mostly holdups from the 'in progress' legacy stuff when it wasn't targeted to core.

          Feels to me like this is close to being ready to being integrated. We don't have to be perfect to get into master (IMO), we can improve in master, its unstable! Speak now about your blockers, else it has my +1 to makes its way there!

          Show
          Dan Poltawski added a comment - Just re-read the comments, I think Tim's initial comments were mostly holdups from the 'in progress' legacy stuff when it wasn't targeted to core. Feels to me like this is close to being ready to being integrated. We don't have to be perfect to get into master (IMO), we can improve in master, its unstable! Speak now about your blockers, else it has my +1 to makes its way there!
          Hide
          Jean-Michel Vedrine added a comment -

          I think Dan is right. If there is no blocker this should go in master so that each component maintainer can verify how component's forms look and problems can be fixed.

          Show
          Jean-Michel Vedrine added a comment - I think Dan is right. If there is no blocker this should go in master so that each component maintainer can verify how component's forms look and problems can be fixed.
          Hide
          Tim Hunt added a comment -

          I think this should go in soon, but we should do two things first:

          1. Implement the heuristics I gave in the comment https://tracker.moodle.org/browse/MDL-30637?focusedCommentId=200757&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-200757, becuase the current heuristics are really bad.

          2. Then we can revert most of the changes to individual form classes that you can see at the end of this diff-stat: https://git.luns.net.uk/moodle.git/commitdiff/master..luns-shortforms-rebased

          3. And, as Andrew suggests, drop the dependency on moodle-core-notification.

          Those are fairly minor clean-ups which we should fix before this gets into main git history.

          Then, if there are other forms that need manually cleaning up, we can deal with that with separate issues later.

          Show
          Tim Hunt added a comment - I think this should go in soon, but we should do two things first: 1. Implement the heuristics I gave in the comment https://tracker.moodle.org/browse/MDL-30637?focusedCommentId=200757&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-200757 , becuase the current heuristics are really bad. 2. Then we can revert most of the changes to individual form classes that you can see at the end of this diff-stat: https://git.luns.net.uk/moodle.git/commitdiff/master..luns-shortforms-rebased 3. And, as Andrew suggests, drop the dependency on moodle-core-notification. Those are fairly minor clean-ups which we should fix before this gets into main git history. Then, if there are other forms that need manually cleaning up, we can deal with that with separate issues later.
          Hide
          Ruslan Kabalin added a comment -

          I agree with Tim, the heuristics he suggested makes sense, I will implement this first, then clean up DisableShortforms usage and it will be ready for integration.

          Show
          Ruslan Kabalin added a comment - I agree with Tim, the heuristics he suggested makes sense, I will implement this first, then clean up DisableShortforms usage and it will be ready for integration.
          Hide
          Ruslan Kabalin added a comment -

          Tim, just need to clarify one bit. You said:

          1- or 2- section forms not collapsible.

          3+ section form forms collapsible, with the first section expanded by default and the rest collapsed.

          Should the second condition be read as:

          3+ section form forms collapsible, with the first section non-collapsible and the rest collapsed.

          The thing is that currently we keep the 'general' section out of collapsible elements, so it does not have expansion control element, or you suggest to keep the first section both collapsible and expanded (so that someone can collapse it if he wants)?

          Show
          Ruslan Kabalin added a comment - Tim, just need to clarify one bit. You said: 1- or 2- section forms not collapsible. 3+ section form forms collapsible, with the first section expanded by default and the rest collapsed. Should the second condition be read as: 3+ section form forms collapsible, with the first section non-collapsible and the rest collapsed. The thing is that currently we keep the 'general' section out of collapsible elements, so it does not have expansion control element, or you suggest to keep the first section both collapsible and expanded (so that someone can collapse it if he wants)?
          Hide
          Ruslan Kabalin added a comment -

          I can see some upcoming issues after this will be integrated, the one I have just came across is mod/assing/mod_form.php, it has four sections with the same id 'general' (which is a bug), they will all be collapsed even though we will skip the first one, the array of collapsible elements will have a single record for all remaining three sections, which at the point of rendering will be applied to the first section as well as it has the same name. I can imagine how many similar issues will pop up

          Show
          Ruslan Kabalin added a comment - I can see some upcoming issues after this will be integrated, the one I have just came across is mod/assing/mod_form.php, it has four sections with the same id 'general' (which is a bug), they will all be collapsed even though we will skip the first one, the array of collapsible elements will have a single record for all remaining three sections, which at the point of rendering will be applied to the first section as well as it has the same name. I can imagine how many similar issues will pop up
          Hide
          Tim Hunt added a comment -

          I think that the first section should be collapsible if the user wants.

          • The possible down-side is that the user might accidentally click to collapse, but I think that is a very minor issue, and easy to fix (click again).
          • The up-side is that it gives the user the freedom to collapse that section if they want, and I think this is quite useful.

          So, that is my thinking. Presumably it would be easy to tweak this in the code later, if we change our mind, so, I think you should take your pick, and get this submitted for integration.

          Show
          Tim Hunt added a comment - I think that the first section should be collapsible if the user wants. The possible down-side is that the user might accidentally click to collapse, but I think that is a very minor issue, and easy to fix (click again). The up-side is that it gives the user the freedom to collapse that section if they want, and I think this is quite useful. So, that is my thinking. Presumably it would be easy to tweak this in the code later, if we change our mind, so, I think you should take your pick, and get this submitted for integration.
          Hide
          Tim Hunt added a comment -

          Upcoming issues are a good reason to get this integrated sooner rather than later. Those things need to be fixed anyway.

          Show
          Tim Hunt added a comment - Upcoming issues are a good reason to get this integrated sooner rather than later. Those things need to be fixed anyway.
          Hide
          Ruslan Kabalin added a comment -

          The branch has been rebased and updated with recent suggestions. Below I sumed up the logic behind forms collapsing:

          If form contains 2 and less sections (headers):

          • Display the form as non-collapsible at all (like before applying this feature).
          • The point above can be overridden if developer marks the section as expanded in form definition (e.g. $mform->setExpanded('foo'));

          If form contains 3 and more sections (headers):

          • always expanding the first section and closing all others by default;
          • always expanding a section with a required element;
          • expanding any section which contains validation errors after submission;
          • expanding any section which was previously open on previous submit (e.g. when adding new choices);
          • expanding the section which is marked as expanded in form definition (e.g. $mform->setExpanded('foo');
          Show
          Ruslan Kabalin added a comment - The branch has been rebased and updated with recent suggestions. Below I sumed up the logic behind forms collapsing: If form contains 2 and less sections (headers): Display the form as non-collapsible at all (like before applying this feature). The point above can be overridden if developer marks the section as expanded in form definition (e.g. $mform->setExpanded('foo')); If form contains 3 and more sections (headers): always expanding the first section and closing all others by default; always expanding a section with a required element; expanding any section which contains validation errors after submission; expanding any section which was previously open on previous submit (e.g. when adding new choices); expanding the section which is marked as expanded in form definition (e.g. $mform->setExpanded('foo');
          Hide
          Tim Hunt added a comment -

          That sounds great.

          I think it is someone else's turn to peer review this code.

          Show
          Tim Hunt added a comment - That sounds great. I think it is someone else's turn to peer review this code.
          Hide
          Andrew Nicols added a comment -

          lib/form/yui/shortforms/shortforms.js

          • The @module yuidoc should read moodle-form-shortforms
          • Can you rename init_shortforms to M.form.shortforms and change the @class from SHORTFORMS to M.form.shortforms
          • The var statement on line 20 is indented incorrectly. It should be:
            var SELECTORS = {
                    XXX: 'span.xxx',
                    YYY: 'div.yyy'
                },
                CSS = {
                    XXX: 'xxx',
                    YYY: 'yyy'
                },
                ATTRS = {};
            
          • Line 65: fieldset.addClass('jsprocessed') - the jsprocessed should be in the CSS var
          • Line 71-72: Can you explain what's going on here - can it be better written as legendelement.insertBefore(headerlink, legendelement.get('firstChild')); or legendelement.insert(headerlink, 'before');
                              headerlink.appendChild(legendelement.get('firstChild'));
                              legendelement.prepend(headerlink);
            
          • Line 76: headerlink.on('click') - it would be more efficient (performance-wise) to write as a Y.delegate outside of the loop
          • There's still an unnecessary dependency on moodle-core-notification. I think it should be possible to entirely drop it.
          • You may wnat to consider moving the anonymous function on the fieldlist.each() to a function on the outside of the initializer. This would allow us to call it on an event trigger in the future if and when we go down the route of allowing for creation of forms in JS popups.

          lib/form/yui/showadvanced/showadvanced.js

          • many of the same as the previous
          • I'd prefer it if we standardised on using M.util.get_string() rather than access M.str directly - we may want to change the API one day, and M.util.get_string() informs of missing strings without throwing an error when the namespace doesn't exist yet.

          More thoughts on the JS

          Is it actually possible to drop the formid entirely? Is there ever a time where form may exist, but not have any collapsable elements and this code not be able to handle it? Likewise with the showadvanced elements.

          lib/formslib.php

          • missing break; statement from the previous case
          • use of camelCase variables. This is against the moodle coding guidelines, but is also congruent with what's already there. If an integrator has thoughts on that case it'd be good to hear them
          • I personally prefer the yui_module calls split onto several lines. It means that any changes to the included params (e.g. new ones) don't suggest that the include itself has changed
          • same for the strings_for_js call
          • collapsable -> collapsible (typo in CSS class names)

          That's all I have on the code for the moment...

          Show
          Andrew Nicols added a comment - lib/form/yui/shortforms/shortforms.js The @module yuidoc should read moodle-form-shortforms Can you rename init_shortforms to M.form.shortforms and change the @class from SHORTFORMS to M.form.shortforms The var statement on line 20 is indented incorrectly. It should be: var SELECTORS = { XXX: 'span.xxx', YYY: 'div.yyy' }, CSS = { XXX: 'xxx', YYY: 'yyy' }, ATTRS = {}; Line 65: fieldset.addClass('jsprocessed') - the jsprocessed should be in the CSS var Line 71-72: Can you explain what's going on here - can it be better written as legendelement.insertBefore(headerlink, legendelement.get('firstChild')); or legendelement.insert(headerlink, 'before'); headerlink.appendChild(legendelement.get('firstChild')); legendelement.prepend(headerlink); Line 76: headerlink.on('click') - it would be more efficient (performance-wise) to write as a Y.delegate outside of the loop There's still an unnecessary dependency on moodle-core-notification. I think it should be possible to entirely drop it. You may wnat to consider moving the anonymous function on the fieldlist.each() to a function on the outside of the initializer. This would allow us to call it on an event trigger in the future if and when we go down the route of allowing for creation of forms in JS popups. lib/form/yui/showadvanced/showadvanced.js many of the same as the previous I'd prefer it if we standardised on using M.util.get_string() rather than access M.str directly - we may want to change the API one day, and M.util.get_string() informs of missing strings without throwing an error when the namespace doesn't exist yet. More thoughts on the JS Is it actually possible to drop the formid entirely? Is there ever a time where form may exist, but not have any collapsable elements and this code not be able to handle it? Likewise with the showadvanced elements. lib/formslib.php missing break; statement from the previous case use of camelCase variables. This is against the moodle coding guidelines, but is also congruent with what's already there. If an integrator has thoughts on that case it'd be good to hear them I personally prefer the yui_module calls split onto several lines. It means that any changes to the included params (e.g. new ones) don't suggest that the include itself has changed same for the strings_for_js call collapsable -> collapsible (typo in CSS class names) That's all I have on the code for the moment...
          Hide
          Ruslan Kabalin added a comment -

          Thanks for revision Andrew!

          lib/form/yui/shortforms/shortforms.js

          The @module yuidoc should read moodle-form-shortforms

          Done.

          Can you rename init_shortforms to M.form.shortforms and change the @class from SHORTFORMS to M.form.shortforms

          Done.

          The var statement on line 20 is indented incorrectly. It should be...

          Done.

          Line 65: fieldset.addClass('jsprocessed') - the jsprocessed should be in the CSS var

          Done.

          Line 71-72: Can you explain what's going on here - can it be better written as legendelement.insertBefore(headerlink, legendelement.get('firstChild')); or legendelement.insert(headerlink, 'before');

          What I do here, the content of legendelement is turned to html link (headerlink). The first of your example will work, but I do not see why the long notation you suggest is better than egendelement.prepend(headerlink); Node.prepend is not deprecated and there is noting wrong with it.

          Line 76: headerlink.on('click') - it would be more efficient (performance-wise) to write as a Y.delegate outside of the loop

          Makes sense, thanks. I have done this.

          There's still an unnecessary dependency on moodle-core-notification. I think it should be possible to entirely drop it.

          True.

          You may wnat to consider moving the anonymous function on the fieldlist.each() to a function on the outside of the initializer.

          Done that.

          lib/form/yui/showadvanced/showadvanced.js

          many of the same as the previous

          Done.

          I'd prefer it if we standardised on using M.util.get_string() rather than access M.str directly

          Done.

          More thoughts on the JS

          Is it actually possible to drop the formid entirely?

          We need to make sure we work within the same formid and update the right state holders elements and so on, there might be more than one from on the same page and there is no guarantee that the second form will have unique element IDs. This is not a hypothetical thing, I saw a real problem with this and pushed 5b70bb8d formid fix to this branch.

          lib/formslib.php

          missing break; statement from the previous case

          Well spotted

          I personally prefer the yui_module calls split onto several lines.

          same for the strings_for_js call

          Hmmm, in this case they are too short to split... I would prefer leaving them as it is :/

          collapsable -> collapsible (typo in CSS class names)

          Changed everywhere

          Thanks!

          Show
          Ruslan Kabalin added a comment - Thanks for revision Andrew! lib/form/yui/shortforms/shortforms.js The @module yuidoc should read moodle-form-shortforms Done. Can you rename init_shortforms to M.form.shortforms and change the @class from SHORTFORMS to M.form.shortforms Done. The var statement on line 20 is indented incorrectly. It should be... Done. Line 65: fieldset.addClass('jsprocessed') - the jsprocessed should be in the CSS var Done. Line 71-72: Can you explain what's going on here - can it be better written as legendelement.insertBefore(headerlink, legendelement.get('firstChild')); or legendelement.insert(headerlink, 'before'); What I do here, the content of legendelement is turned to html link (headerlink). The first of your example will work, but I do not see why the long notation you suggest is better than egendelement.prepend(headerlink); Node.prepend is not deprecated and there is noting wrong with it. Line 76: headerlink.on('click') - it would be more efficient (performance-wise) to write as a Y.delegate outside of the loop Makes sense, thanks. I have done this. There's still an unnecessary dependency on moodle-core-notification. I think it should be possible to entirely drop it. True. You may wnat to consider moving the anonymous function on the fieldlist.each() to a function on the outside of the initializer. Done that. lib/form/yui/showadvanced/showadvanced.js many of the same as the previous Done. I'd prefer it if we standardised on using M.util.get_string() rather than access M.str directly Done. More thoughts on the JS Is it actually possible to drop the formid entirely? We need to make sure we work within the same formid and update the right state holders elements and so on, there might be more than one from on the same page and there is no guarantee that the second form will have unique element IDs. This is not a hypothetical thing, I saw a real problem with this and pushed 5b70bb8d formid fix to this branch. lib/formslib.php missing break; statement from the previous case Well spotted I personally prefer the yui_module calls split onto several lines. same for the strings_for_js call Hmmm, in this case they are too short to split... I would prefer leaving them as it is :/ collapsable -> collapsible (typo in CSS class names) Changed everywhere Thanks!
          Hide
          Ruslan Kabalin added a comment -

          Removed some extra module dependencies. Ready for the second run of revision.

          Show
          Ruslan Kabalin added a comment - Removed some extra module dependencies. Ready for the second run of revision.
          Hide
          Tim Hunt added a comment -

          Sorry, just spotted some things:

          1. You deleted the old strings

          $string['hideadvanced'] = 'Hide advanced';
          $string['showadvanced'] = 'Show advanced';

          but actually they are still used in a lot of places.

          2. I also spotted some things that codechecker would complain about (and I don't just mean the camelCase where you are following the existing style).

          3. In serveral places, you add $CFG to a global statement, but you don't seem to use it anywhere.

          Apart from that, lets get this submitted for integration.

          Show
          Tim Hunt added a comment - Sorry, just spotted some things: 1. You deleted the old strings $string ['hideadvanced'] = 'Hide advanced'; $string ['showadvanced'] = 'Show advanced'; but actually they are still used in a lot of places. 2. I also spotted some things that codechecker would complain about (and I don't just mean the camelCase where you are following the existing style). 3. In serveral places, you add $CFG to a global statement, but you don't seem to use it anywhere. Apart from that, lets get this submitted for integration.
          Hide
          Andrew Nicols added a comment -

          Agreed - the code is looking pretty good aside from those areas. Anything else can be handled post-integration IMO.

          Show
          Andrew Nicols added a comment - Agreed - the code is looking pretty good aside from those areas. Anything else can be handled post-integration IMO.
          Hide
          Ruslan Kabalin added a comment -

          Thanks Tim!

          1. You deleted the old strings

          Reverted.

          2. I also spotted some things that codechecker would complain about (and I don't just mean the camelCase where you are following the existing style).

          Can't test, codechecker says 'Well done!' with 0 files checked when I enter lib/formslib.php. May be a bug in it.

          3. In serveral places, you add $CFG to a global statement, but you don't seem to use it anywhere.

          True, that is probably remained from the time when shortforms had config setting. Removed.

          All these fixes are in a single commit

          If there are no more comments, then I squash all commits into two (one for shortforms, one for advanced things) and submit for integration.

          Show
          Ruslan Kabalin added a comment - Thanks Tim! 1. You deleted the old strings Reverted. 2. I also spotted some things that codechecker would complain about (and I don't just mean the camelCase where you are following the existing style). Can't test, codechecker says 'Well done!' with 0 files checked when I enter lib/formslib.php. May be a bug in it. 3. In serveral places, you add $CFG to a global statement, but you don't seem to use it anywhere. True, that is probably remained from the time when shortforms had config setting. Removed. All these fixes are in a single commit If there are no more comments, then I squash all commits into two (one for shortforms, one for advanced things) and submit for integration.
          Hide
          Tim Hunt added a comment -

          You changed more than one file! (Codechecker can check JS too.)

          Show
          Tim Hunt added a comment - You changed more than one file! (Codechecker can check JS too.)
          Hide
          Ruslan Kabalin added a comment -

          Yeah, I changed more than one, but what I am trying to say, I was not able to run it for at least one of them.

          Show
          Ruslan Kabalin added a comment - Yeah, I changed more than one, but what I am trying to say, I was not able to run it for at least one of them.
          Hide
          Ruslan Kabalin added a comment -

          Squashed all commits into few ones, ready for integration.

          Show
          Ruslan Kabalin added a comment - Squashed all commits into few ones, ready for integration.
          Hide
          Damyon Wiese 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.

          Cheers!

          Show
          Damyon Wiese 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. Cheers!
          Hide
          Damyon Wiese added a comment -

          Big thanks to everyone for working on/reviewing/discussing/improving this issue. I have pushed this to integration/master now and we can all start showing it off to pleasant ohhs and ahhs.

          If anyone sees a form that is behaving oddly or could be improved to work with this feature, please add a new issue and link it to this one.

          I added a linked issue to make sure the hiding/showing of the collapsible form elements are announced to screen readers. I also added a commit to cleanup some comments. I also added deprecated versions of the functions that were removed from MoodleQuickForm to aid developers.

          Show
          Damyon Wiese added a comment - Big thanks to everyone for working on/reviewing/discussing/improving this issue. I have pushed this to integration/master now and we can all start showing it off to pleasant ohhs and ahhs. If anyone sees a form that is behaving oddly or could be improved to work with this feature, please add a new issue and link it to this one. I added a linked issue to make sure the hiding/showing of the collapsible form elements are announced to screen readers. I also added a commit to cleanup some comments. I also added deprecated versions of the functions that were removed from MoodleQuickForm to aid developers.
          Hide
          Dan Poltawski added a comment -

          Do we have another issue for proposed changes to forms?

          For example, I seem to remember at Lancaster that adjusted the 'forum' form, so that these were all in their own section too:
          Subscription mode
          Read tracking for this forum?
          Maximum attachment size
          Maximum number of attachments

          Show
          Dan Poltawski added a comment - Do we have another issue for proposed changes to forms? For example, I seem to remember at Lancaster that adjusted the 'forum' form, so that these were all in their own section too: Subscription mode Read tracking for this forum? Maximum attachment size Maximum number of attachments
          Hide
          Ruslan Kabalin added a comment -

          Dan: the separate issue for this is probably the best approach, otherwise we will be loosing the track of good ideas.

          Show
          Ruslan Kabalin added a comment - Dan: the separate issue for this is probably the best approach, otherwise we will be loosing the track of good ideas.
          Hide
          Damyon Wiese added a comment -

          Just adding a note here to say that this is now causing behat failures - David will add a fix in MDL-37873.

          Show
          Damyon Wiese added a comment - Just adding a note here to say that this is now causing behat failures - David will add a fix in MDL-37873 .
          Hide
          Michael de Raadt added a comment -

          I've been looking forward to this.

          Show
          Michael de Raadt added a comment - I've been looking forward to this.
          Hide
          Frédéric Massart added a comment -

          I think a larger padding would be better . See screenshot.

          Show
          Frédéric Massart added a comment - I think a larger padding would be better . See screenshot.
          Hide
          Michael de Raadt added a comment -

          Test result: Success!

          This worked well. I tried this with and without JS on a number of activity, resource and block settings pages. I tried the Choice activity in FF, IE and Chrome.

          I suspect there will be a number of regressions, relating to usability, following this issue. I found one in resources and others have mentioned others. I don't think this should prevent the integration of this change.

          The structure of this issue is odd. This issue has open sub-tasks and we are testing it. A META issue should be created and this issue, as well as the sub-tasks of this issue, should become sub-tasks of the META. To shift an issue to a new META, convert it to an issue, then back to a sub-task.

          Show
          Michael de Raadt added a comment - Test result: Success! This worked well. I tried this with and without JS on a number of activity, resource and block settings pages. I tried the Choice activity in FF, IE and Chrome. I suspect there will be a number of regressions, relating to usability, following this issue. I found one in resources and others have mentioned others. I don't think this should prevent the integration of this change. The structure of this issue is odd. This issue has open sub-tasks and we are testing it. A META issue should be created and this issue, as well as the sub-tasks of this issue, should become sub-tasks of the META. To shift an issue to a new META, convert it to an issue, then back to a sub-task.
          Hide
          Dan Poltawski added a comment -

          I pushed a fix for the bad padding.

          Fred/Barbara - the looks were considered, but then we at HQ spend over a year ignoring the patch and then asked for lots of changes. Inevitably regressions happen. The important point is that we detect and fix them, it is not because we don't value the work you put into this.

          Show
          Dan Poltawski added a comment - I pushed a fix for the bad padding. Fred/Barbara - the looks were considered, but then we at HQ spend over a year ignoring the patch and then asked for lots of changes. Inevitably regressions happen. The important point is that we detect and fix them, it is not because we don't value the work you put into this.
          Hide
          Barbara Ramiro added a comment -

          No big deal Dan. And thanks for the fix.

          Functionality + Good looks = moodlelicious

          Show
          Barbara Ramiro added a comment - No big deal Dan. And thanks for the fix. Functionality + Good looks = moodlelicious
          Hide
          Tim Hunt added a comment -

          As Michael says, we need to create a META for follow-up issues, to get this fully "moodlelicious" (is that even a word) before 2.5 is finished.

          Show
          Tim Hunt added a comment - As Michael says, we need to create a META for follow-up issues, to get this fully "moodlelicious" (is that even a word) before 2.5 is finished.
          Hide
          Ruslan Kabalin added a comment -

          Created meta issue: MDL-38012

          Show
          Ruslan Kabalin added a comment - Created meta issue: MDL-38012
          Hide
          Damyon Wiese added a comment -

          Congratulations this fix has been added to Moodle!

          You may want to dedicate this issue to someone special on this Valentines day.

          Thanks!

          Show
          Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!
          Hide
          Ruslan Kabalin added a comment -

          Good idea Damyon, I dedicate it to my wife Evgenia

          Show
          Ruslan Kabalin added a comment - Good idea Damyon, I dedicate it to my wife Evgenia
          Hide
          David Mudrak added a comment -

          Regression detected MDL-38243, should be easy to fix.

          Show
          David Mudrak added a comment - Regression detected MDL-38243 , should be easy to fix.
          Hide
          Colin Chambers added a comment -

          HI guys,

          Just trying to implement the setExpanded() feature on the question type forms. Tried $mform->setExpanded('answerhdr', true); inside edit_question_form.php. No joy. When I trace the call order I find that setExpand gets called again in the accept method of formslib and seems to collapse it.

          Hence. either I'm calling from the wrong place, calling it incorrectly or calling the function. Or this is a bug. I assume it's not the latter. Any tips?

          c

          Show
          Colin Chambers added a comment - HI guys, Just trying to implement the setExpanded() feature on the question type forms. Tried $mform->setExpanded('answerhdr', true); inside edit_question_form.php. No joy. When I trace the call order I find that setExpand gets called again in the accept method of formslib and seems to collapse it. Hence. either I'm calling from the wrong place, calling it incorrectly or calling the function. Or this is a bug. I assume it's not the latter. Any tips? c
          Hide
          Frédéric Massart added a comment -

          Hi Colin,

          setExpanded is being buggy at the moment, MDL-38435 took care of restoring its functionality. Although, if you are using a repeated element, it's within its options that you have to set 'expanded' to True.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Colin, setExpanded is being buggy at the moment, MDL-38435 took care of restoring its functionality. Although, if you are using a repeated element, it's within its options that you have to set 'expanded' to True. Cheers, Fred
          Hide
          Colin Chambers added a comment -

          Excellent. Just pulled latest master and setExpanded now works. MDL-38345 seems to have fixed it.

          Frederic, thanks for that tip.

          Show
          Colin Chambers added a comment - Excellent. Just pulled latest master and setExpanded now works. MDL-38345 seems to have fixed it. Frederic, thanks for that tip.
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as this is being documented during the documentation of the new display settings in 2.5

          Show
          Mary Cooch added a comment - Removing docs_required label as this is being documented during the documentation of the new display settings in 2.5

            People

            • Votes:
              21 Vote for this issue
              Watchers:
              31 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: