Moodle
  1. Moodle
  2. MDL-38012 META: Simplify moodle forms
  3. MDL-38044

Add "expand all" and "collapse all" controls to simplified forms

    Details

    • Testing Instructions:
      Hide
      1. Go to the course page settings
        • Confirm that there are 2 buttons to collapse and expand all the sections
        • Confirm that when all sections are expanded it's not possible to use "Expand all"
        • Confirm that when all sections are collapsed it's not possible to use "Collapse all"
        • Confirm that the buttons are hidden when JS is turned off
      2. Use the attach file test_forms.php and play with it.
        • Confirm that a form with only one collapsible section does NOT display the buttons. (MDL-38455 needs to be integrated to test this)
        • Confirm that setting an element as Advanced, and clicking "Show more/less" works as expected
      Show
      Go to the course page settings Confirm that there are 2 buttons to collapse and expand all the sections Confirm that when all sections are expanded it's not possible to use "Expand all" Confirm that when all sections are collapsed it's not possible to use "Collapse all" Confirm that the buttons are hidden when JS is turned off Use the attach file test_forms.php and play with it. Confirm that a form with only one collapsible section does NOT display the buttons. ( MDL-38455 needs to be integrated to test this) Confirm that setting an element as Advanced, and clicking "Show more/less" works as expected
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-38044-master
    • Rank:
      47837

      Description

      A suggestion has come to me from a couple of people after seeing the simplified forms where most sections are collapsed. The suggestion is to add a control to expand all sections. There could also possibly be a collapse all sections control, although that may not be necessary.

      1. test_forms.php
        7 kB
        Frédéric Massart

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Fred and Alex: I've added you as watchers as you were both interested in this.

          Show
          Michael de Raadt added a comment - Fred and Alex: I've added you as watchers as you were both interested in this.
          Hide
          Dan Poltawski added a comment -

          Might be good to add Ruslan to the issue.. since hes the one who originally developed it!

          Show
          Dan Poltawski added a comment - Might be good to add Ruslan to the issue.. since hes the one who originally developed it!
          Hide
          Frédéric Massart added a comment -

          Here is a patch that adds the Collapse/Expand all buttons, please note that:

          • I didn't add the buttons to the bottom of the form, because if you collapse or expand from there, you lose track of where you are as the page dimension changes without scrolling anywhere, so it's more confusing to the user.
          • The choice of 2 buttons, to disable them, to position on the right, etc... is based on Barbara's comment about usability.
          • I didn't want to add the buttons dynamically in JS for 2 reasons:
            • I didn't want to page to flicker when the buttons are added
            • I wanted them to be stylable (used in renderers)
          • The choice of adding a new raw template of the collapsible buttons in formslib is based on the fact that using addElement('button') was not flexible enough to do what I needed.
            • It would add a new hidden fieldset (which inherits styles, which are not selectable for collapsible buttons only)
            • It was not easy to add them at the top of the form
          • The buttons are disabled by default so that it leaves time for the JS to load and enable them. Otherwise a click on them would act on the form.
          Show
          Frédéric Massart added a comment - Here is a patch that adds the Collapse/Expand all buttons, please note that: I didn't add the buttons to the bottom of the form, because if you collapse or expand from there, you lose track of where you are as the page dimension changes without scrolling anywhere, so it's more confusing to the user. The choice of 2 buttons, to disable them, to position on the right, etc... is based on Barbara's comment about usability. I didn't want to add the buttons dynamically in JS for 2 reasons: I didn't want to page to flicker when the buttons are added I wanted them to be stylable (used in renderers) The choice of adding a new raw template of the collapsible buttons in formslib is based on the fact that using addElement('button') was not flexible enough to do what I needed. It would add a new hidden fieldset (which inherits styles, which are not selectable for collapsible buttons only) It was not easy to add them at the top of the form The buttons are disabled by default so that it leaves time for the JS to load and enable them. Otherwise a click on them would act on the form.
          Hide
          Andrew Nicols added a comment -

          Some quick notes now - fuller PR coming later:

          • form = formorbtn.ancestor(SELECTORS.form, true); will do what you want there without the if test (line 141)
          • 112: Y.one (not Y.Node.on)
          • 69: I prefer all var definitions together (personal pref)
          • 69: Probably worth testing that the form exists before delegating on it: if (!form) { Y.log("Some note about the form not existing"); return; }
          • 134: var declaration in for definition
          • 134: Better written as fieldlist.each(function(node) { .... }

            );

          May be a few other things. Have you run through jshint?

          Show
          Andrew Nicols added a comment - Some quick notes now - fuller PR coming later: form = formorbtn.ancestor(SELECTORS.form, true); will do what you want there without the if test (line 141) 112: Y.one (not Y.Node.on) 69: I prefer all var definitions together (personal pref) 69: Probably worth testing that the form exists before delegating on it: if (!form) { Y.log("Some note about the form not existing"); return; } 134: var declaration in for definition 134: Better written as fieldlist.each(function(node) { .... } ); May be a few other things. Have you run through jshint?
          Hide
          Ruslan Kabalin added a comment -

          Looked through your patches Fred, just want to say thank you for working on it. I like the idea of not processing last header separately and thus reducing the code duplication.

          Show
          Ruslan Kabalin added a comment - Looked through your patches Fred, just want to say thank you for working on it. I like the idea of not processing last header separately and thus reducing the code duplication.
          Hide
          Andrew Nicols added a comment -

          Here's my initial peer review.

          I just want to have another look at the formslib stuff before I complete my review though. I think it's all fine, I just want to go over it with a fine-toothed comb.

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [N] Testing
          [Y] Security
          [?] Documentation
          [Y] Git
          [Y] Sanity check

          This looks good to me and simplifies that hack that Tim and I wrote (Kudos).

          On the testing front, could you expand the part 2 instructions.
          Also, I think there's a mistake. If there's only one section, the buttons don't appear, but you ask to confirm that they do.

          Documentation-wise I think it's fine - I can't think of anything extra to document but sjust thought I'd raise it.

          Show
          Andrew Nicols added a comment - Here's my initial peer review. I just want to have another look at the formslib stuff before I complete my review though. I think it's all fine, I just want to go over it with a fine-toothed comb. [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [N] Testing [Y] Security [?] Documentation [Y] Git [Y] Sanity check This looks good to me and simplifies that hack that Tim and I wrote (Kudos). On the testing front, could you expand the part 2 instructions. Also, I think there's a mistake. If there's only one section, the buttons don't appear, but you ask to confirm that they do. Documentation-wise I think it's fine - I can't think of anything extra to document but sjust thought I'd raise it.
          Hide
          Andrew Nicols added a comment -

          Yup - it all looks good to me. Feel free to submit for Integration when ready.

          Show
          Andrew Nicols added a comment - Yup - it all looks good to me. Feel free to submit for Integration when ready.
          Hide
          Frédéric Massart added a comment -

          (Note for self and integrators: The selector FORM is not needed any more, I forgot to remove it)

          Show
          Frédéric Massart added a comment - (Note for self and integrators: The selector FORM is not needed any more, I forgot to remove it)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Reopening this:

          1) is marked as blocked by MDL-38455 (that is open).
          2) it's conflicting badly (5 files).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Reopening this: 1) is marked as blocked by MDL-38455 (that is open). 2) it's conflicting badly (5 files). Ciao
          Hide
          Tim Hunt added a comment -

          It is not blocked by "Review default collapsing statuses" who thought that? "expand all" and "collapse all" is useful functionality whatever the review of other forms shows.

          Show
          Tim Hunt added a comment - It is not blocked by "Review default collapsing statuses" who thought that? "expand all" and "collapse all" is useful functionality whatever the review of other forms shows.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Frédéric Massart added a comment -

          It was blocked by MDL-38455 because some variables were renamed and this was using the renamed one. I have rebased on master, removed the other commits and updated the branch. Pushing for integration again.

          Show
          Frédéric Massart added a comment - It was blocked by MDL-38455 because some variables were renamed and this was using the renamed one. I have rebased on master, removed the other commits and updated the branch. Pushing for integration again.
          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.

          Thanks!

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

          Thanks Fred + reviewers, commenters,

          Some thoughts about this change are:

          I don't like that the buttons are actual buttons - they look too similar to the submit buttons used to submit the form. I think they should be links. I'll raise that as a separate issue and see if anyone agrees with me.

          Integrated to master only (obviously).

          Show
          Damyon Wiese added a comment - Thanks Fred + reviewers, commenters, Some thoughts about this change are: I don't like that the buttons are actual buttons - they look too similar to the submit buttons used to submit the form. I think they should be links. I'll raise that as a separate issue and see if anyone agrees with me. Integrated to master only (obviously).
          Hide
          Damyon Wiese added a comment -

          Vote for MDL-38681 if you think they should be links!

          Show
          Damyon Wiese added a comment - Vote for MDL-38681 if you think they should be links!
          Hide
          Rossiani Wijaya added a comment -

          This is working as expected.

          Tested for master

          Test passed.

          Show
          Rossiani Wijaya added a comment - This is working as expected. Tested for master Test passed.
          Hide
          Damyon Wiese added a comment -

          Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).

          Show
          Damyon Wiese added a comment - Thanks for your hard work. This issue has been integrated upstream and is now available via git (and in some hours, via mirrors and downloads).
          Hide
          Mary Cooch added a comment -

          Removing docs_required label as I have been documenting this as I've been documenting each activity/resource with new settings.

          Show
          Mary Cooch added a comment - Removing docs_required label as I have been documenting this as I've been documenting each activity/resource with new settings.

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: