Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-35615

"Use default section name" checkbox and "Section name" fields should be inline

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2, 2.3.5, 2.4.2, 2.5
    • Fix Version/s: 2.3.6, 2.4.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      1. Turn editing on for a course
      2. Edit a course section

      On edit page, make sure the alignment for checkbox and label for 'use default section name' are inline with section name input text. Also make sure the functionality works as it should.

      Show
      Turn editing on for a course Edit a course section On edit page, make sure the alignment for checkbox and label for 'use default section name' are inline with section name input text. Also make sure the functionality works as it should.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Moodle usually make use of a single row for related forms elements like in the examples I displayed in the attachments: example 1 and 2.
      Why do the "Summary of section" is out of standards?
      My suggestion is to change what I reported in current.png into expected.png.

      The fix is trivial:
      in HEAD/course/editsection_form.php
      change

          function definition() {
       
              $mform  = $this->_form;
              $course = $this->_customdata['course'];
              $mform->addElement('checkbox', 'usedefaultname', get_string('sectionusedefaultname'));
              $mform->setDefault('usedefaultname', true);
       
              $mform->addElement('text', 'name', get_string('sectionname'), array('size'=>'30'));
              $mform->setType('name', PARAM_TEXT);
              $mform->disabledIf('name','usedefaultname','checked');
       
              /// Prepare course and the editor
       
              $mform->addElement('editor', 'summary_editor', get_string('summary'), null, $this->_customdata['editoroptions']);
              $mform->addHelpButton('summary_editor', 'summary');
              $mform->setType('summary_editor', PARAM_RAW);
       
              $mform->addElement('hidden', 'id');
              $mform->setType('id', PARAM_INT);
       
              $mform->_registerCancelButton('cancel');
          }

      to

          function definition() {
       
              $mform  = $this->_form;
       
              $course = $this->_customdata['course'];
       
              $elementgroup = array();
              $elementgroup[] = $mform->createElement('text', 'name', '', array('size'=>'30'));
              $elementgroup[] = $mform->createElement('checkbox', 'usedefaultname', '', get_string('sectionusedefaultname'));
              $mform->addGroup($elementgroup, 'name_group', get_string('sectionname'), ' ', false);
       
              $mform->setDefault('usedefaultname', true);
              $mform->setType('name', PARAM_TEXT);
              $mform->disabledIf('name','usedefaultname','checked');
       
              /// Prepare course and the editor
       
              $mform->addElement('editor', 'summary_editor', get_string('summary'), null, $this->_customdata['editoroptions']);
              $mform->addHelpButton('summary_editor', 'summary');
              $mform->setType('summary_editor', PARAM_RAW);
       
              $mform->addElement('hidden', 'id');
              $mform->setType('id', PARAM_INT);
       
              $mform->_registerCancelButton('cancel');
          }

        Gliffy Diagrams

          Attachments

          1. current.png
            current.png
            26 kB
          2. example1.png
            example1.png
            21 kB
          3. example2.png
            example2.png
            9 kB
          4. expected.png
            expected.png
            28 kB

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that and providing a fix.

            When this issue is resolved, we should probably look for other similar examples.

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that and providing a fix. When this issue is resolved, we should probably look for other similar examples.
            Hide
            daniss Daniele Cordella added a comment -

            Thanks Rossiani!
            It is nice to see you accepted my suggestion. Really thank you.

            Show
            daniss Daniele Cordella added a comment - Thanks Rossiani! It is nice to see you accepted my suggestion. Really thank you.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Daniele,

            No worries.

            Thank you for providing the patch.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Daniele, No worries. Thank you for providing the patch.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Daniele and Rossie,

            Patch looks good, a minor things you might want to consider before pushing it for integration:

            1. Space in array https://github.com/rwijaya/moodle/compare/moodle:master...MDL-35615#L0R25

            [y] Syntax
            [y] Output
            [y] Whitespace
            [-] Language
            [-] Databases
            [y] Testing
            [-] Security
            [-] Documentation
            [y] Git
            [y] Sanity check

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Daniele and Rossie, Patch looks good, a minor things you might want to consider before pushing it for integration: Space in array https://github.com/rwijaya/moodle/compare/moodle:master...MDL-35615#L0R25 [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
            Hide
            daniss Daniele Cordella added a comment -

            I am not sure I correctly understand.
            Space in array? Which array?
            Do you mean:
            $elementgroup[] = $mform->createElement('text', 'name', '', array('size'=>'30'));
            to
            $elementgroup[] = $mform->createElement('text', 'name', '', array('size' => '30'));
            ?

            Show
            daniss Daniele Cordella added a comment - I am not sure I correctly understand. Space in array? Which array? Do you mean: $elementgroup[] = $mform->createElement('text', 'name', '', array('size'=>'30')); to $elementgroup[] = $mform->createElement('text', 'name', '', array('size' => '30')); ?
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Yes Daniele.
            As per coding style, there should be space after key and before value.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Yes Daniele. As per coding style, there should be space after key and before value.
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Hi Daniele,

            Yes, that is what Raj referring to.

            I will update the patch and send it for integration.

            Show
            rwijaya Rossiani Wijaya added a comment - Hi Daniele, Yes, that is what Raj referring to. I will update the patch and send it for integration.
            Hide
            daniss Daniele Cordella added a comment -

            Cool! Thanks.

            Show
            daniss Daniele Cordella added a comment - Cool! Thanks.
            Hide
            daniss Daniele Cordella added a comment -

            cd /your/local/moodle/head
            egrep -rn '=>[^\ ]|[^\ ]=>' *

            I get a lot of occurrences

            Show
            daniss Daniele Cordella added a comment - cd /your/local/moodle/head egrep -rn '=> [^\ ] | [^\ ] =>' * I get a lot of occurrences
            Hide
            rwijaya Rossiani Wijaya added a comment -

            Fixed spaces within the array. Patch updated.

            Sending for integration review.

            Show
            rwijaya Rossiani Wijaya added a comment - Fixed spaces within the array. Patch updated. Sending for integration review.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Yes Daniele, there are a lot of occurrences where this is not followed.
            But coding style guide has been updated since then and any new code should follow this.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Yes Daniele, there are a lot of occurrences where this is not followed. But coding style guide has been updated since then and any new code should follow this.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

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

            TIA and ciao

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

            thanks, thats been integrated into 23, 24 and master.

            Show
            nebgor Aparup Banerjee added a comment - thanks, thats been integrated into 23, 24 and master.
            Hide
            fred Frédéric Massart added a comment -

            Test passed. I'm looking forward for some standard classes that we could apply on such elements, the checkbox sticked to the label is not that fancy.

            Show
            fred Frédéric Massart added a comment - Test passed. I'm looking forward for some standard classes that we could apply on such elements, the checkbox sticked to the label is not that fancy.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

            Thanks!

            PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks! PS: Yay, legacy template messages. Yes, you're ok, we don't have CVS anymore!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  18/Mar/13