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

custommenu should allow override by theme

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2
    • Fix Version/s: 2.3
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      can only be tested by someone with ability to modify code.

      in your custom theme settings.php add this (replacing 'theme_afterburner' with the name of your theme = eg 'theme_theloop1' - this displays the custommenu settings on the themes own settings page.

      $name = 'theme_afterburner/custommenuitems';
      $title = get_string('custommenuitems', 'admin');
      $description = get_string('configcustommenuitems', 'admin');
      $default = '';
      $setting = new admin_setting_configtextarea($name, $title, $description, $default);
      $settings->add($setting);

      then in your theme file theme/layout/default.php find this line:

      $custommenu = $OUTPUT->custom_menu();

      and replace it with this:

      $custommenu = $OUTPUT->custom_menu($PAGE->theme->settings->custommenuitems);

      • this tells your theme to use the custommenu defined by the theme instead of using the "global" one.

      then go to your custom themes "settings" page and set up the menu items as you would like - load the theme in your browser and check to see if the custommenu items are set as expected.

      Show
      can only be tested by someone with ability to modify code. in your custom theme settings.php add this (replacing 'theme_afterburner' with the name of your theme = eg 'theme_theloop1' - this displays the custommenu settings on the themes own settings page. $name = 'theme_afterburner/custommenuitems'; $title = get_string('custommenuitems', 'admin'); $description = get_string('configcustommenuitems', 'admin'); $default = ''; $setting = new admin_setting_configtextarea($name, $title, $description, $default); $settings->add($setting); then in your theme file theme/layout/default.php find this line: $custommenu = $OUTPUT->custom_menu(); and replace it with this: $custommenu = $OUTPUT->custom_menu($PAGE->theme->settings->custommenuitems); this tells your theme to use the custommenu defined by the theme instead of using the "global" one. then go to your custom themes "settings" page and set up the menu items as you would like - load the theme in your browser and check to see if the custommenu items are set as expected.
    • Affected Branches:
      MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      master_MDL-31043

      Description

      the function custom_menu in outputrenderers.php should allow a different string to be passed so that different themes can set their own custom menus

      here's a possible replacement:
      public function custom_menu($custommenuitems='') {
      global $CFG;
      if (empty($custommenuitems) && !empty($CFG->custommenuitems))

      { $custommenuitems = $CFG->custommenuitems; }

      if (empty($custommenuitems))

      { return ''; }

      $custommenu = new custom_menu($custommenuitems, current_language());
      return $this->render_custom_menu($custommenu);
      }

      this means that in a themes default.php we could do something like this:
      $custommenu = $OUTPUT->custom_menu($PAGE->theme->settings->custommenuitems);

        Gliffy Diagrams

          Activity

          Hide
          danmarsden Dan Marsden added a comment -

          would be nice for this to go into 2.2Stable and master - thanks.

          Show
          danmarsden Dan Marsden added a comment - would be nice for this to go into 2.2Stable and master - thanks.
          Hide
          salvetore Michael de Raadt added a comment -

          This should normally go to peer review, I think.

          I had a look at the code and it's quite a simple change and looks good to me, so letting this go through to the keeper.

          Show
          salvetore Michael de Raadt added a comment - This should normally go to peer review, I think. I had a look at the code and it's quite a simple change and looks good to me, so letting this go through to the keeper.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Hi Dan,

          Thanks for these changes, they have been integrated now.

          I spent a bit of time thinking about this issue.
          There are a couple of things that struck me:

          1/ There was a plan at some point in the future to implement a proper interface to the set and manage custom menu items. This would hopefully make it easier to define a more in-depth and dynamic custom menu.

          2/ This is already technically possible by overriding the core_renderer within a theme and using the function you now have as part of your changes or even just looking directly at $PAGE->theme->settings->custommenuitems within the overridden renderer.

          My justification for integrating this presently is that in regards to 1\ it hasn't happened yet and likely won't happen for some time yet, and 2\ this is easier.
          However I have decided seeing as 2.2.1 has now been released that this will only end up in master (2.3) sorry.

          Cheers
          Sam

          Show
          samhemelryk Sam Hemelryk added a comment - Hi Dan, Thanks for these changes, they have been integrated now. I spent a bit of time thinking about this issue. There are a couple of things that struck me: 1/ There was a plan at some point in the future to implement a proper interface to the set and manage custom menu items. This would hopefully make it easier to define a more in-depth and dynamic custom menu. 2/ This is already technically possible by overriding the core_renderer within a theme and using the function you now have as part of your changes or even just looking directly at $PAGE->theme->settings->custommenuitems within the overridden renderer. My justification for integrating this presently is that in regards to 1\ it hasn't happened yet and likely won't happen for some time yet, and 2\ this is easier. However I have decided seeing as 2.2.1 has now been released that this will only end up in master (2.3) sorry. Cheers Sam
          Hide
          danmarsden Dan Marsden added a comment -

          thanks Sam - master only is fine by me.

          Show
          danmarsden Dan Marsden added a comment - thanks Sam - master only is fine by me.
          Hide
          lazydaisy Mary Evans added a comment -

          @Dan,

          I've just seen this tracker, and was not sure how it would work as I have not tested it yet, but reading through the code again, does this mean the custommenu can be set differently in a theme thus overriding the one set Globally? If this is the case this is perfect for those who want a different menu when enrolled on a particular course, and so providing a theme has such a settings page then and allowing course themes, an Admin could set such a menu.

          If I am correct in my assumptions, then this is super brill! Thanks
          Mary

          Show
          lazydaisy Mary Evans added a comment - @Dan, I've just seen this tracker, and was not sure how it would work as I have not tested it yet, but reading through the code again, does this mean the custommenu can be set differently in a theme thus overriding the one set Globally? If this is the case this is perfect for those who want a different menu when enrolled on a particular course, and so providing a theme has such a settings page then and allowing course themes, an Admin could set such a menu. If I am correct in my assumptions, then this is super brill! Thanks Mary
          Hide
          lazydaisy Mary Evans added a comment -

          @Moodle Forum Theme Designers INK
          Just added you guys as I thought this would be of interest!
          Cheers
          Mary

          Show
          lazydaisy Mary Evans added a comment - @Moodle Forum Theme Designers INK Just added you guys as I thought this would be of interest! Cheers Mary
          Hide
          dwahl Daniel Wahl added a comment -

          One suggestion I would have for this since the goal is to override/extend/modify the custommenu in the site admin, would be to store the default string as the value stored in the admin setting (admin/custommenuitems) rather than as a null string.

          Show
          dwahl Daniel Wahl added a comment - One suggestion I would have for this since the goal is to override/extend/modify the custommenu in the site admin, would be to store the default string as the value stored in the admin setting (admin/custommenuitems) rather than as a null string.
          Hide
          lazydaisy Mary Evans added a comment -

          Good thinking Daniel!

          Show
          lazydaisy Mary Evans added a comment - Good thinking Daniel!
          Hide
          roelmann Richard Oelmann added a comment -

          +1 for Daniel's comment
          Thanks for the heads up on this Mary. It's ideal for a couple of scenarios I'm looking at right now menus by category/course - solved using this, and child themes forced to a course

          Show
          roelmann Richard Oelmann added a comment - +1 for Daniel's comment Thanks for the heads up on this Mary. It's ideal for a couple of scenarios I'm looking at right now menus by category/course - solved using this, and child themes forced to a course
          Hide
          danmarsden Dan Marsden added a comment -

          I'm not sure what Daniel is suggesting here - are you suggesting a change to the code going into core - or the code that sets the default on your own custom theme?

          Show
          danmarsden Dan Marsden added a comment - I'm not sure what Daniel is suggesting here - are you suggesting a change to the code going into core - or the code that sets the default on your own custom theme?
          Hide
          dwahl Daniel Wahl added a comment - - edited

          sorry I have the gift of speaking in a convoluted way:

          Changing the theme default value to match the core value.

          Show
          dwahl Daniel Wahl added a comment - - edited sorry I have the gift of speaking in a convoluted way: Changing the theme default value to match the core value.
          Hide
          danmarsden Dan Marsden added a comment -

          cool - you can implement it however you want in the themes themselves - I only added the code above in the testing instructions to help the QA person to test. (this patch doesn't change any themes - just the core renderer functions to allow us to override it)

          Show
          danmarsden Dan Marsden added a comment - cool - you can implement it however you want in the themes themselves - I only added the code above in the testing instructions to help the QA person to test. (this patch doesn't change any themes - just the core renderer functions to allow us to override it)
          Hide
          lazydaisy Mary Evans added a comment -

          @Dan

          Thanks for this novel way the custommenu will work in Moodle 2.3 - I've got it working and it works great!

          I think what Daniel is saying, is that it would be good to see the actual default data in the textarea rather than an empty box. However I am not sure how one can do this with the default menu data. Where is it stored anyway? And how would you implement Daniel's idea if you can be done?

          Show
          lazydaisy Mary Evans added a comment - @Dan Thanks for this novel way the custommenu will work in Moodle 2.3 - I've got it working and it works great! I think what Daniel is saying, is that it would be good to see the actual default data in the textarea rather than an empty box. However I am not sure how one can do this with the default menu data. Where is it stored anyway? And how would you implement Daniel's idea if you can be done?
          Hide
          danmarsden Dan Marsden added a comment -

          I guess you'd change this bit in the first block:
          $default = '';
          to this:
          $default = $CFG->custommenuitems;

          but - this might cause confusion for admins - if they set up the main admin level custommenu then install a theme that uses this, then update the main admin level menu it won't 'update' the custom theme's menu - it might be better to leave blank so that the main admin menu is always used unless the admin specifically sets the custommenu in your custom theme?

          We have a client using this that has multiple schools on a single install - each school has a course category with custom theme set at the category level - they all have their own main sites and want to link to their own library pages etc.

          Show
          danmarsden Dan Marsden added a comment - I guess you'd change this bit in the first block: $default = ''; to this: $default = $CFG->custommenuitems; but - this might cause confusion for admins - if they set up the main admin level custommenu then install a theme that uses this, then update the main admin level menu it won't 'update' the custom theme's menu - it might be better to leave blank so that the main admin menu is always used unless the admin specifically sets the custommenu in your custom theme? We have a client using this that has multiple schools on a single install - each school has a course category with custom theme set at the category level - they all have their own main sites and want to link to their own library pages etc.
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Just dropping in to agree with Dan who I think has hit the nail on the head with his thoughts on the confusion that may be caused by `copying` the custom menu when changing settings in both places.

          Show
          samhemelryk Sam Hemelryk added a comment - Just dropping in to agree with Dan who I think has hit the nail on the head with his thoughts on the confusion that may be caused by `copying` the custom menu when changing settings in both places.
          Hide
          lazydaisy Mary Evans added a comment -

          I agree with you both. I have just tried that, and it is very confusing, and looks untidy too.

          Thanks

          Show
          lazydaisy Mary Evans added a comment - I agree with you both. I have just tried that, and it is very confusing, and looks untidy too. Thanks
          Hide
          gerry Gerard Caulfield added a comment -

          Overriding global theme menu settings via the in-theme menu settings works. Test passed.

          Show
          gerry Gerard Caulfield added a comment - Overriding global theme menu settings via the in-theme menu settings works. Test passed.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          This virus has been spread upstream, everybody will be infected soon. Congrats, you did it!

          Closing, ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - This virus has been spread upstream, everybody will be infected soon. Congrats, you did it! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                25/Jun/12