Moodle
  1. Moodle
  2. MDL-31043

custommenu should allow override by theme

    Details

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

      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);

        Activity

        Hide
        Dan Marsden added a comment -

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

        Show
        Dan Marsden added a comment - would be nice for this to go into 2.2Stable and master - thanks.
        Hide
        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
        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
        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
        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
        Dan Marsden added a comment -

        thanks Sam - master only is fine by me.

        Show
        Dan Marsden added a comment - thanks Sam - master only is fine by me.
        Hide
        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
        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
        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
        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
        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
        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
        Mary Evans added a comment -

        Good thinking Daniel!

        Show
        Mary Evans added a comment - Good thinking Daniel!
        Hide
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        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
        Gerard Caulfield added a comment -

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

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

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

        Closing, ciao

        Show
        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: