Moodle
  1. Moodle
  2. MDL-35615

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      44343

      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');
          }
      
      1. current.png
        26 kB
      2. example1.png
        21 kB
      3. example2.png
        9 kB
      4. expected.png
        28 kB

        Activity

        Hide
        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
        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
        Daniele Cordella added a comment -

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

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

        Hi Daniele,

        No worries.

        Thank you for providing the patch.

        Show
        Rossiani Wijaya added a comment - Hi Daniele, No worries. Thank you for providing the patch.
        Hide
        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
        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
        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
        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
        Rajesh Taneja added a comment -

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

        Show
        Rajesh Taneja added a comment - Yes Daniele. As per coding style, there should be space after key and before value.
        Hide
        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
        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
        Daniele Cordella added a comment -

        Cool! Thanks.

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

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

        I get a lot of occurrences

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

        Fixed spaces within the array. Patch updated.

        Sending for integration review.

        Show
        Rossiani Wijaya added a comment - Fixed spaces within the array. Patch updated. Sending for integration review.
        Hide
        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
        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
        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
        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
        Aparup Banerjee added a comment -

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

        Show
        Aparup Banerjee added a comment - thanks, thats been integrated into 23, 24 and master.
        Hide
        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
        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
        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
        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: