Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Forms Library, Usability
    • Labels:
    • Testing Instructions:
      Hide

      Easy test

      1. Go to the course settings page
        • Make sure all the sections are collapsible
        • Make sure all but the first section are collapsed
      2. On the page to add a new blog entry
        • Make sure there are only 2 sections
        • Make sure the second one tags is expanded

      Pro test

      1. Use the attached file test_forms.php
      2. Play with the form
      3. Extend some of them by adding:
        • Fields (addElement)
        • Rules (addRule)
        • Expanding/collapsing (setExpanded)
      4. Make sure
        • An error on a field will ALWAYS expand its section
        • A require field will ALWAYS expand its section
        • The sections are collapsed/expanded according to the state in which they were when the form was submitted (except with errors, required)
      Show
      Easy test Go to the course settings page Make sure all the sections are collapsible Make sure all but the first section are collapsed On the page to add a new blog entry Make sure there are only 2 sections Make sure the second one tags is expanded Pro test Use the attached file test_forms.php Play with the form Extend some of them by adding: Fields (addElement) Rules (addRule) Expanding/collapsing (setExpanded) Make sure An error on a field will ALWAYS expand its section A require field will ALWAYS expand its section The sections are collapsed/expanded according to the state in which they were when the form was submitted (except with errors, required)
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-38455-master-int
    • Rank:
      48415

      Description

      I have a proposal to:

      • Prevent the first section from being collapsible in all forms
      • Always enable the collapsibility (currently it's disabled when the form has less than 3 sections)
      • When the form has only 2 sections, expand the 2nd one by default
      1. test_forms.php
        7 kB
        Frédéric Massart

        Issue Links

          Activity

          Hide
          Frédéric Massart added a comment -

          Here is the list of what I have done:

          • The first section is not-collapsible by default;
          • The second section is expanded by default if the form has 2 sections;
          • Any section can be set to non-collapsible;
          • The collapsed state by setExpanded can override the state in which the section was when it was submitted by the user;
          • A header containing a required field or one that has an error will never be collapsed.
          Show
          Frédéric Massart added a comment - Here is the list of what I have done: The first section is not-collapsible by default; The second section is expanded by default if the form has 2 sections; Any section can be set to non-collapsible; The collapsed state by setExpanded can override the state in which the section was when it was submitted by the user; A header containing a required field or one that has an error will never be collapsed.
          Hide
          Ankit Agarwal added a comment -

          Hi Fred,
          I reviewed this commit as you asked https://github.com/FMCorz/moodle/commit/e137bcad0b363aceaba8f2880ec3b54ffff6db84

          1. Can you explain why we are doing this:-
             if (empty($headername)) {
            +            return;
            +        } 
            
          2. isset($this->_collapsibleElements[$headername]) && !$this->_collapsibleElements[$headername]
            

            Probabily can be simplified to

            !empty($this->_collapsibleElements[$headername])
            
          3. This is minor, feel free to ignore, but there is a conflict between the expanded state value between setExpanded and $_collapsibleStates which is not that great for code readability
          4. The setExpanded() doc says "The method is applicable to header elements only." And we are doing
            if ($element->getType() == 'header') {
            } else if ($anyrequiredorerror) {
            $this->setExpanded($headername, true, true);
            } 
            

            Rest looks good. Thanks

          Show
          Ankit Agarwal added a comment - Hi Fred, I reviewed this commit as you asked https://github.com/FMCorz/moodle/commit/e137bcad0b363aceaba8f2880ec3b54ffff6db84 Can you explain why we are doing this:- if (empty($headername)) { + return; + } isset($this->_collapsibleElements[$headername]) && !$this->_collapsibleElements[$headername] Probabily can be simplified to !empty($this->_collapsibleElements[$headername]) This is minor, feel free to ignore, but there is a conflict between the expanded state value between setExpanded and $_collapsibleStates which is not that great for code readability The setExpanded() doc says "The method is applicable to header elements only." And we are doing if ($element->getType() == 'header') { } else if ($anyrequiredorerror) { $this->setExpanded($headername, true, true); } Rest looks good. Thanks
          Hide
          Frédéric Massart added a comment -

          Thanks for your review Ankit!

          1/ If the header (fieldset) does not have a proper name, no ID will be generated for it, and it will lead to potential Javascript errors. In order to setExpanded or setCollapsible, the section requires a valid name;
          2/ No it cannot, because if the value is not set, then I assume that it is collapsible, if it is set, but false, then it's not collapsible. I know it could be simpler to read, but I tried to simplify that the best I could.
          3/ I did not introduce those names, I feel the same way.
          4/ And that is true, it's only applicable to header elements. The loop you're extracting a bit of code from is checking the elements within a header. If one of those elements match some rules, then we will expand the header. (Note $headername passed to the function)

          Cheers, I am pushing this for integration!

          Show
          Frédéric Massart added a comment - Thanks for your review Ankit! 1/ If the header (fieldset) does not have a proper name, no ID will be generated for it, and it will lead to potential Javascript errors. In order to setExpanded or setCollapsible, the section requires a valid name; 2/ No it cannot, because if the value is not set, then I assume that it is collapsible, if it is set, but false, then it's not collapsible. I know it could be simpler to read, but I tried to simplify that the best I could. 3/ I did not introduce those names, I feel the same way. 4/ And that is true, it's only applicable to header elements. The loop you're extracting a bit of code from is checking the elements within a header. If one of those elements match some rules, then we will expand the header. (Note $headername passed to the function) Cheers, I am pushing this for integration!
          Hide
          Aparup Banerjee added a comment -

          Hi, was it the whole branch or just commit https://github.com/FMCorz/moodle/commit/e137bcad0b363aceaba8f2880ec3b54ffff6db84 that was peer-reviewed?

          also the first header is still collapsing here. (stopping - gtg scrum course)

          Show
          Aparup Banerjee added a comment - Hi, was it the whole branch or just commit https://github.com/FMCorz/moodle/commit/e137bcad0b363aceaba8f2880ec3b54ffff6db84 that was peer-reviewed? also the first header is still collapsing here. (stopping - gtg scrum course)
          Hide
          Damyon Wiese added a comment -

          Stealing from Apu (just picking in order).

          Show
          Damyon Wiese added a comment - Stealing from Apu (just picking in order).
          Hide
          Damyon Wiese added a comment -

          Hi Fred,

          Just reviewing this patch - and I see the same as Aparup - the first section is collapsible.

          I also am not a fan of the arrays of booleans. One suggestion would be a single array with const int states:

          EXPANDED_NOT_COLLAPSIBLE
          EXPANDED_FORCED
          EXPANDED
          COLLAPSED

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Fred, Just reviewing this patch - and I see the same as Aparup - the first section is collapsible. I also am not a fan of the arrays of booleans. One suggestion would be a single array with const int states: EXPANDED_NOT_COLLAPSIBLE EXPANDED_FORCED EXPANDED COLLAPSED Thanks, Damyon
          Hide
          Aparup Banerjee added a comment -

          Thanks Damyon, just here talking to Fred here (in training) - Barbara and Fred spoke and agreed for usability to have the first section collapsible.

          Show
          Aparup Banerjee added a comment - Thanks Damyon, just here talking to Fred here (in training) - Barbara and Fred spoke and agreed for usability to have the first section collapsible.
          Hide
          Damyon Wiese added a comment -

          Updated testing instructions re requirements change.

          Show
          Damyon Wiese added a comment - Updated testing instructions re requirements change.
          Hide
          Frédéric Massart added a comment - - edited

          Thanks guys, yes I had forgotten to update the testing instructions, my bad. I agree with you Damyon, I should have introduced those constants. Unfortunately I won't have time to patch this issue before Wednesday, so I'll leave it up to you.

          Thinking about it, I'm not sure it'll add a lot of clarification. Actually, we need a constant "COLLAPSIBLE_UNSET", because you still want a section to be collapsible but to inherit the defaults. In our case, the default depends on multiple criteria, and so we could end up with more complex if statements, just to have an array of integers.

          I haven't thought that really in depth, but I don't think it's a good idea to force the expanded/collapsed status when setting a section as collapsible. Though I entirely agree that the setCollapsible method is now useless as all sections are collapsible by default... And it's unlikely that you'll require a non-collapsible section, but who knows...

          Actually... perhaps it'd be even better to forget about all the changes adding that method, so that 3rd party plugins and core developers wouldn't be tempted to disable the collapsibility of some sections, and ruin the user experience. Barbara might have an opinion on that. (Do we want to have more control than All sections are collapsible and None of the section are?)

          I still won't have time to work on the patch, but as for now I think it's probably better to rework the patch and remote the, now useless, new method.

          Up to you, still .

          Show
          Frédéric Massart added a comment - - edited Thanks guys, yes I had forgotten to update the testing instructions, my bad. I agree with you Damyon, I should have introduced those constants. Unfortunately I won't have time to patch this issue before Wednesday, so I'll leave it up to you. Thinking about it, I'm not sure it'll add a lot of clarification. Actually, we need a constant "COLLAPSIBLE_UNSET", because you still want a section to be collapsible but to inherit the defaults. In our case, the default depends on multiple criteria, and so we could end up with more complex if statements, just to have an array of integers. I haven't thought that really in depth, but I don't think it's a good idea to force the expanded/collapsed status when setting a section as collapsible. Though I entirely agree that the setCollapsible method is now useless as all sections are collapsible by default... And it's unlikely that you'll require a non-collapsible section, but who knows... Actually... perhaps it'd be even better to forget about all the changes adding that method, so that 3rd party plugins and core developers wouldn't be tempted to disable the collapsibility of some sections, and ruin the user experience. Barbara might have an opinion on that. (Do we want to have more control than All sections are collapsible and None of the section are ?) I still won't have time to work on the patch, but as for now I think it's probably better to rework the patch and remote the, now useless, new method. Up to you, still .
          Hide
          Ralf Hilgenstock added a comment -

          Hi Frederic

          I've made a walkthrough all modules and course settings. This are my findings.

          Generally, its a great step forward. It works nice. I found that some modules needs changes where settings are located. May be we need some additional tracker entries for this.

          General settings - This area should be limited to real general settings. In some modules the general settings level has to much issues. Please move them to a seperate area: i.e. glossary, database, forum, lesson,
          Common module settings - should be closed per default

          The last section is generally open per default. It should be closed.

          folder
          Should the file upload area in folder module be open per default. Its possible to create a folder but to keep it empty, but the standard process will be that files are added from beginning. The scorm module is defined in a way that file upload is visible all the time.

          ims package
          Differently to SCORM here the content area is closed by default. That makes no sense.

          page
          a typical problem here is that users add their content to the description area and not in the content field. Can we solve this problem here? I don't know what a best solution is.

          Course settings
          Move the following items into a seperate area. They are not fundamentally required.

          • Course start date,
          • News items to show
          • Show gradebook to students, Show activity reports
          • Maximum upload size
            Close role renaming per default.

          You wrote that any section can be set to non collapsible. Where is this setting? Haven't found it.

          Show
          Ralf Hilgenstock added a comment - Hi Frederic I've made a walkthrough all modules and course settings. This are my findings. Generally, its a great step forward. It works nice. I found that some modules needs changes where settings are located. May be we need some additional tracker entries for this. General settings - This area should be limited to real general settings. In some modules the general settings level has to much issues. Please move them to a seperate area: i.e. glossary, database, forum, lesson, Common module settings - should be closed per default The last section is generally open per default. It should be closed. folder Should the file upload area in folder module be open per default. Its possible to create a folder but to keep it empty, but the standard process will be that files are added from beginning. The scorm module is defined in a way that file upload is visible all the time. ims package Differently to SCORM here the content area is closed by default. That makes no sense. page a typical problem here is that users add their content to the description area and not in the content field. Can we solve this problem here? I don't know what a best solution is. Course settings Move the following items into a seperate area. They are not fundamentally required. Course start date, News items to show Show gradebook to students, Show activity reports Maximum upload size Close role renaming per default. You wrote that any section can be set to non collapsible. Where is this setting? Haven't found it.
          Hide
          Frédéric Massart added a comment -

          Hi Ralf,

          thanks for this feedback. It will be very helpful from tomorrow, when I'll start going around all the modules forms, and reformat them a bit. Some of the problems you raised already have issues associated to them: Last section is not collapsed (MDL-38340), or Reformatting the course settings (MDL-38415). You can have a look at them in the META issue MDL-38012.

          The setting for non-collapsible section is not integrated yet, it's part of the patch in this issue. And I am not sure it's going to be integrated at all at the end.

          Cheers,
          Fred

          Show
          Frédéric Massart added a comment - Hi Ralf, thanks for this feedback. It will be very helpful from tomorrow, when I'll start going around all the modules forms, and reformat them a bit. Some of the problems you raised already have issues associated to them: Last section is not collapsed ( MDL-38340 ), or Reformatting the course settings ( MDL-38415 ). You can have a look at them in the META issue MDL-38012 . The setting for non-collapsible section is not integrated yet, it's part of the patch in this issue. And I am not sure it's going to be integrated at all at the end. Cheers, Fred
          Hide
          Damyon Wiese added a comment -

          Thanks for the feedback Fred, I'll let you do some more work on this patch and re-submit it instead of rushing it through.

          Show
          Damyon Wiese added a comment - Thanks for the feedback Fred, I'll let you do some more work on this patch and re-submit it instead of rushing it through.
          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
          Helen Foster added a comment -

          Ralf, thanks again for your feedback - it's very helpful.

          Regarding the IMS content package content section being closed, I've just created MDL-38552 for it.

          Regarding the page resource, I know just what you mean by 'users add their content to the description area and not in the content field'! Perhaps a solution would be to make the description text box a lot smaller than the page content text box, say just big enough to fit one or two sentences in. The description text box could actually be made smaller for EVERY activity and resource to save space on the page, since as with all text boxes the size can be changed by a user by dragging the bottom right corner. If you agree with my suggested solution, please create a new tracker issue for it.

          Show
          Helen Foster added a comment - Ralf, thanks again for your feedback - it's very helpful. Regarding the IMS content package content section being closed, I've just created MDL-38552 for it. Regarding the page resource, I know just what you mean by 'users add their content to the description area and not in the content field'! Perhaps a solution would be to make the description text box a lot smaller than the page content text box, say just big enough to fit one or two sentences in. The description text box could actually be made smaller for EVERY activity and resource to save space on the page, since as with all text boxes the size can be changed by a user by dragging the bottom right corner. If you agree with my suggested solution, please create a new tracker issue for it.
          Hide
          Frédéric Massart added a comment -

          Sending back for peer review. The patch has been simplified a lot.

          • No more possibility to disable the collapsibility of some sections
          • The first section is expanded by default
          • The second is expanded too if the form has only 2 sections

          Thanks everyone!

          Show
          Frédéric Massart added a comment - Sending back for peer review. The patch has been simplified a lot. No more possibility to disable the collapsibility of some sections The first section is expanded by default The second is expanded too if the form has only 2 sections Thanks everyone!
          Hide
          Ankit Agarwal added a comment -

          Thanks Fred. Patch is looking simpler and better now. Pushing for integration.
          Thanks

          Show
          Ankit Agarwal added a comment - Thanks Fred. Patch is looking simpler and better now. Pushing for integration. Thanks
          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
          Dan Poltawski added a comment -

          Integrated to master. Thanks Fred.

          Show
          Dan Poltawski added a comment - Integrated to master. Thanks Fred.
          Hide
          Aparup Banerjee added a comment -

          quick suggestion : MDL-38859

          Show
          Aparup Banerjee added a comment - quick suggestion : MDL-38859
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred,

          Works as mentioned.

          Show
          Rajesh Taneja added a comment - Thanks Fred, Works as mentioned.
          Hide
          Dan Poltawski added a comment -

          Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking.

          line 1289 of \lib\changes.php: call to debugging()
          line 281 of \lib\are.php: call to moodleform->detectMissingThanks()
          line 202 of \lib\now.php: call to moodleform->_is_poor_form()
          line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

          Show
          Dan Poltawski added a comment - Did you remember to call thankDevelopers() for 'this_weeks_work'? Defaulting to PARAM_SHODDY thanking. line 1289 of \lib\changes.php: call to debugging() line 281 of \lib\are.php: call to moodleform->detectMissingThanks() line 202 of \lib\now.php: call to moodleform->_is_poor_form() line 73 of \course\upstream.php: call to moodleform->forgetingToThank()

            People

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

              Dates

              • Created:
                Updated:
                Resolved: