Details

    • Type: Sub-task
    • Status: Closed
    • Priority: 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

      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

        Gliffy Diagrams

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

          Issue Links

            Activity

            Hide
            fred 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
            fred 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_frenz 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_frenz 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
            fred 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
            fred 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
            nebgor 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
            nebgor 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 Damyon Wiese added a comment -

            Stealing from Apu (just picking in order).

            Show
            damyon Damyon Wiese added a comment - Stealing from Apu (just picking in order).
            Hide
            damyon 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 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
            nebgor 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
            nebgor 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 Damyon Wiese added a comment -

            Updated testing instructions re requirements change.

            Show
            damyon Damyon Wiese added a comment - Updated testing instructions re requirements change.
            Hide
            fred 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
            fred 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
            ralfh 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
            ralfh 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
            fred 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
            fred 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 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 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 CiBoT added a comment -

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

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            tsala 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
            tsala 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
            fred 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
            fred 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_frenz Ankit Agarwal added a comment -

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

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

            Integrated to master. Thanks Fred.

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

            quick suggestion : MDL-38859

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

            Thanks Fred,

            Works as mentioned.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Fred, Works as mentioned.
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  14/May/13