Allow General Section to be Numbered #23

5882wolverine · 2017-07-19T12:08:20Z

Addresses issue #22.

…orrection to incorrect variable name

…course home page.

… home page highlighting.

… highlighted.

5882wolverine · 2017-07-19T12:09:49Z

Hi,

I've just submitted a pull request now :)

Thanks,

M

mudrd8mz

Thanks for the pull request. Please see some first things I've noticed when looking at the patch. Also note, it is probably worth rebasing the branch and melting some commits together.

mudrd8mz · 2017-08-11T12:03:39Z

block_course_contents.php

@@ -164,15 +164,41 @@ public function get_content() {
                $title = $format->get_section_name($section);
            }

+            if ( ($i == 0) && (!empty($globalconfig->display_course_link)) ) {


There seems to be some extra whitespace next to outer brackets.

I added that for clarity, but I'm happy to correct in the next pull request.

Yeah but Moodle has its coding style which we follow in plugins, too. Please see https://docs.moodle.org/dev/Coding_style

mudrd8mz · 2017-08-11T12:07:18Z

block_course_contents.php

@@ -164,15 +164,41 @@ public function get_content() {
                $title = $format->get_section_name($section);
            }

+            if ( ($i == 0) && (!empty($globalconfig->display_course_link)) ) {


We should not check the globalconfig here. The global config should be used for setting the default values. Each block instance should have these settings available (via edit_form.php file).

mudrd8mz · 2017-08-11T12:10:33Z

block_course_contents.php

            $odd = $r % 2;
-            if ($format->is_section_current($section)) {


This seems to remove the existing feature of highlighting the current section?

From our testing it seems to highlight fine?

I just re-checked and I believe there was some misunderstanding. Your has changed the meaning of the class "current" to match the section the user currently clicked to. But originally this was a section highlighted as the "current" one by the teacher of the course.

Anyway, I realized your intention and I agree that highlighting the currently selected section should be more prominent than highlighting the current one. I am adding an extra change that introduces a new CSS class "selected" which will use the original "current" styling. The "current" section will be still highlighted but not that much.

This is an example of the result of my fix: in the following example, the user is viewing the section "Topic 4" and the teacher has marked the "Topic 2" as the current one. Your patch did not highlight the "Topic 2" at all, my new version will show it in bold:

mudrd8mz · 2017-08-11T12:12:04Z

block_course_contents.php

            } else {
                $text .= html_writer::start_tag('li', array('class' => 'section-item r'.$odd));
            }

-            if ($i == 0) {
-                // Never enumerate the section number 0.
+            $enumeratesection0 = (!empty($globalconfig->enumerate_section_0) ? true : false);


Again, this should be instance-level setting. Also, I usually prefer explicit if/then block rather than this.

mudrd8mz · 2017-08-11T12:13:10Z

block_course_contents.php

            if ($enumerate) {
-                $title = html_writer::span($i, 'section-number').' '.html_writer::span($title, 'section-title');
+                $title = html_writer::span($sectionnumber, 'section-number'). ' '
+                        . html_writer::span('&nbsp;' . $title, 'section-title');


What is the point of the   here?

For a little extra spacing between the number and section title.

I am removing it and adding an extra space properly via CSS.

mudrd8mz · 2017-08-11T12:16:51Z

lang/en/block_course_contents.php

+$string['pluginname'] = 'Course contents';
+$string['config_enumerate_section_0'] = 'Enumerate section 0';


This could be reworded into more human language. Something like "Include the general section" or "Start enumerating with the general section" or so.

mudrd8mz · 2017-08-11T12:17:16Z

lang/en/block_course_contents.php

+$string['config_display_course_link'] = 'Display course page link';
+$string['config_display_course_link_desc'] = 'Display course home page link before all sections at the top.';
+$string['config_display_course_link_text'] = 'Display Course page link anchor text';


Is there a use-case for not using the course name?

Mainly because the course name might be too long.

mudrd8mz · 2017-08-11T12:18:21Z

settings.php

@@ -41,6 +41,21 @@
        $enumerate
    ));

+    // Enumerate section 0.
+    $settings->add(new admin_setting_configcheckbox('block_course_contents/enumerate_section_0',
+            get_string('config_enumerate_section_0', 'block_course_contents'),


These should be lang_string() instances too.

… for enumerate general section 0 and display course page link in same manner as autotitle setting (allow overrides at instance level).

5882wolverine · 2017-11-21T14:50:24Z

Hi David,

I've submitted a pull request again so you should have another commit from today in there. Hope the changes are ok.

I've made global and per-instance settings for the settings enumerate section 0 and display course link in the same way you did for auto title.

Thanks,

M

mudrd8mz · 2018-01-30T20:45:35Z

Thanks for the contribution. I've melt your commits into a single one (9 were a bit too much for a change like this) and added some minor changes on top of it.

I merged the final version into the master branch.

I will appreciate if you can review the result and let me know you are happy with it so that I can release it as a new version of the plugin. Thanks in advance.

Manoj Solanki added 7 commits July 7, 2017 11:06

Address issue mudrd8mz#22 raised in original repository for this plugin.

f6dd322

Minor corrections following code checker run.

e6be39b

Added section number bullet point before course page link and minor c…

4239b96

…orrection to incorrect variable name

Correction to ensure correct section is highlighted when in a section.

f32eb3a

Minor fix to ensure course page home link is highlighted when on the …

61d88b7

…course home page.

Ensure general section highlighted correctly without affecting course…

3132427

… home page highlighting.

Minor updates to formatting and consistent look for each section when…

17ef39a

… highlighted.

mudrd8mz requested changes Aug 11, 2017

View reviewed changes

Manoj Solanki added 2 commits November 20, 2017 11:36

Merge remote-tracking branch 'upstream/master'

b6f00cf

Updates following original pull request. Made instance level settings…

527de25

… for enumerate general section 0 and display course page link in same manner as autotitle setting (allow overrides at instance level).

mudrd8mz closed this Jan 30, 2018

		$string['pluginname'] = 'Course contents';
		$string['config_enumerate_section_0'] = 'Enumerate section 0';

Allow General Section to be Numbered #23

Allow General Section to be Numbered #23

Uh oh!

Conversation

5882wolverine commented Jul 19, 2017

Uh oh!

5882wolverine commented Jul 19, 2017

Uh oh!

mudrd8mz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

5882wolverine commented Nov 21, 2017

Uh oh!

mudrd8mz commented Jan 30, 2018

Uh oh!

Uh oh!