Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      • Have at least two languages installed.
      • Use Clean theme.
      • Add some custom menu items in Appearance > Themes > Theme settings.
      • TEST that 'Language' is displayed as the last entry in your custom menu.
      • Remove the custom menu items you added to the Theme settings page.
      • Now TEST that 'Language' is displayed as the only entry in your custom menu.
      • Use the admin setting to hide the language menu.
      • Repeat the above tests (both with and without custom menu items).
      • TEST that NO language menu appears in either case.
      Show
      Have at least two languages installed. Use Clean theme. Add some custom menu items in Appearance > Themes > Theme settings. TEST that 'Language' is displayed as the last entry in your custom menu. Remove the custom menu items you added to the Theme settings page. Now TEST that 'Language' is displayed as the only entry in your custom menu. Use the admin setting to hide the language menu. Repeat the above tests (both with and without custom menu items). TEST that NO language menu appears in either case.
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-38979-master
    • Rank:
      49082

      Description

      When I try to get a language menu on the front page the Moodle does not show this with the Bootstrap theme.
      I wonder that I get a language menu when I add some text into the field "custom menu items".
      Ralf

        Issue Links

          Activity

          Hide
          David Scotson added a comment -

          The language menu is added to the end of the custom menu. A code merging mistake meant that the language menu wasn't displayed unless you had entries in the custom menu.

          https://github.com/bmbrands/theme_bootstrap/commit/56bd79de3c2865716ea7f6607783ba4cf42660a3

          While fixing that I noticed that we seem to be ignoring the admin setting about whether to show the language menu or not. A current workaround is to limit the language menu to one item. Presumably we should check that setting in the same place as the above fix.

          Show
          David Scotson added a comment - The language menu is added to the end of the custom menu. A code merging mistake meant that the language menu wasn't displayed unless you had entries in the custom menu. https://github.com/bmbrands/theme_bootstrap/commit/56bd79de3c2865716ea7f6607783ba4cf42660a3 While fixing that I noticed that we seem to be ignoring the admin setting about whether to show the language menu or not. A current workaround is to limit the language menu to one item. Presumably we should check that setting in the same place as the above fix.
          Hide
          David Scotson added a comment -
          Show
          David Scotson added a comment - This commit makes it respect the $cfg->langmenu setting: https://github.com/bmbrands/theme_bootstrap/commit/bc7b32f6a0cfef49affccf3ddb43a575cbe32df7
          Hide
          Mary Evans added a comment -

          @David: You also need to add the option for language menu for the Frontpage layout in bootstrap/config.php

              'frontpage' => array(
                  'file' => 'frontpage.php',
                  'regions' => array('side-pre', 'side-post'),
                  'defaultregion' => 'side-pre',
                  'options' => array('langmenu'=>true),        
              ),
          
          Show
          Mary Evans added a comment - @David: You also need to add the option for language menu for the Frontpage layout in bootstrap/config.php 'frontpage' => array( 'file' => 'frontpage.php', 'regions' => array('side-pre', 'side-post'), 'defaultregion' => 'side-pre', 'options' => array('langmenu'=> true ), ),
          Hide
          David Scotson added a comment -

          I'm just guessing but I assume that was taken out by Bas because the lang menu was instead being injected into the custom menu? It seems to appear everywhere for me despite that setting not being there.

          Just checked layout/general.php in Base and Bootstrap. That setting is checked in Base but isn't used in Bootstrap. I'm not sure if it's appropriate to check it or not. Why wouldn't you want the menu displayed on some screens?

          Show
          David Scotson added a comment - I'm just guessing but I assume that was taken out by Bas because the lang menu was instead being injected into the custom menu? It seems to appear everywhere for me despite that setting not being there. Just checked layout/general.php in Base and Bootstrap. That setting is checked in Base but isn't used in Bootstrap. I'm not sure if it's appropriate to check it or not. Why wouldn't you want the menu displayed on some screens?
          Hide
          Mary Evans added a comment -

          The patch needs to be a branch off Moodle master. Assigning this to you as you know more about Bootstrap.

          Show
          Mary Evans added a comment - The patch needs to be a branch off Moodle master. Assigning this to you as you know more about Bootstrap.
          Hide
          Mary Evans added a comment - - edited

          David is this fixed as I see a language menu in the menubar. Or is this becasue I have set my site to use custom menu?

          If this is the case then if no menu is set perhaps Bootstrap should add the lang menu as normal inside headermenu?

          Show
          Mary Evans added a comment - - edited David is this fixed as I see a language menu in the menubar. Or is this becasue I have set my site to use custom menu? If this is the case then if no menu is set perhaps Bootstrap should add the lang menu as normal inside headermenu?
          Hide
          Ralf Krause added a comment -

          I tested the current 2.5beta (20130418).
          The language menu is not shown when the custom head menu is not shown.
          The language menu is always shown when the custom menu is shown ... even the language menu is switched off.

          Show
          Ralf Krause added a comment - I tested the current 2.5beta (20130418). The language menu is not shown when the custom head menu is not shown. The language menu is always shown when the custom menu is shown ... even the language menu is switched off.
          Hide
          Mary Evans added a comment -

          Thanks Ralf.

          I'll see what I can do with this.

          Show
          Mary Evans added a comment - Thanks Ralf. I'll see what I can do with this.
          Hide
          Frédéric Massart added a comment -

          Thanks for fixing this David,

          While your patch seems to do the trick, I am not sure we should head in that direction. I think copying the logic from lang_menu() to custom_menu() creates 2 problems:

          1. It makes it harder for core developers to maintain the code. The same code is in 2 different places, which are not even related to each other in the first place.
          2. As Bootstrap is a base, other themers will have to override 2 methods in order to customize their language menu.

          I know this patch does not introduce the issue, I understand the reason why it had been done like that in the first place, and surely we should have a renderable object for the language menu, as well as render_lang_menu(), but it's now too late to introduce them in core now.

          An alternative, though not the best, is to use lang_menu() to return an array of languages which you can then use in custom_menu() too. The above problems still remain, but at least the logic is constrained in the same method.

          Regardless of my comments above, strictly about the patch you provided, here are a really few minor comments:

          1. 93: $this->page->course != SITEID and !empty($this->page->course->lang) should be encapsulated in between brackets.
          2. 105: Needs identation.
          3. Your commit message could use the component name: http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages

          Many thanks!

          Fred

          Show
          Frédéric Massart added a comment - Thanks for fixing this David, While your patch seems to do the trick, I am not sure we should head in that direction. I think copying the logic from lang_menu() to custom_menu() creates 2 problems: It makes it harder for core developers to maintain the code. The same code is in 2 different places, which are not even related to each other in the first place. As Bootstrap is a base, other themers will have to override 2 methods in order to customize their language menu. I know this patch does not introduce the issue, I understand the reason why it had been done like that in the first place, and surely we should have a renderable object for the language menu, as well as render_lang_menu(), but it's now too late to introduce them in core now. An alternative, though not the best, is to use lang_menu() to return an array of languages which you can then use in custom_menu() too. The above problems still remain, but at least the logic is constrained in the same method. Regardless of my comments above, strictly about the patch you provided, here are a really few minor comments: 93: $this->page->course != SITEID and !empty($this->page->course->lang) should be encapsulated in between brackets. 105: Needs identation. Your commit message could use the component name: http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages Many thanks! Fred
          Hide
          David Scotson added a comment -

          Hi Fred,

          I agree the the duplication is unfortunate, but I think Bas was right to put the languages in the main custom/Bootstrap menu. The collapsing at small sizes just didn't work otherwise, and mobile is the big focus for Moodle adopting this theme. There's more than this area where things could be improved by changing a bit of core, rather than working around things in the theme.

          I've added those brackets and pulled that line up onto the previous. I know it's a long line, but given the indention it would still be about the same width on it's own line. I also added bootstrapbase to the commit message.

          I think it's ready for integration if someone can put it forward.

          Show
          David Scotson added a comment - Hi Fred, I agree the the duplication is unfortunate, but I think Bas was right to put the languages in the main custom/Bootstrap menu. The collapsing at small sizes just didn't work otherwise, and mobile is the big focus for Moodle adopting this theme. There's more than this area where things could be improved by changing a bit of core, rather than working around things in the theme. I've added those brackets and pulled that line up onto the previous. I know it's a long line, but given the indention it would still be about the same width on it's own line. I also added bootstrapbase to the commit message. I think it's ready for integration if someone can put it forward.
          Hide
          Frédéric Massart added a comment -

          Thanks David! Pushing forward.

          Show
          Frédéric Massart added a comment - Thanks David! Pushing forward.
          Hide
          Michael de Raadt added a comment -

          I think this is implied in the testing instructions, but perhaps they should explicitly state that custom menu items should be used. There should probably also be a test that checks for the language menu when there are no custom menu items.

          Show
          Michael de Raadt added a comment - I think this is implied in the testing instructions, but perhaps they should explicitly state that custom menu items should be used. There should probably also be a test that checks for the language menu when there are no custom menu items.
          Hide
          David Scotson added a comment -

          Clarifying testing instructions based on above feedback.

          Show
          David Scotson added a comment - Clarifying testing instructions based on above feedback.
          Hide
          Dan Poltawski added a comment -

          I have created an issue, and added a commit for the code duplication: MDL-39565

          Show
          Dan Poltawski added a comment - I have created an issue, and added a commit for the code duplication: MDL-39565
          Hide
          Dan Poltawski added a comment -

          Integrated and tested during integration. Thanks David.

          We can address that duplication properly in the linked issue.

          Show
          Dan Poltawski added a comment - Integrated and tested during integration. Thanks David. We can address that duplication properly in the linked issue.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Did you think this day was not going to arrive ever?

          Your patience has been rewarded, yay, sent upstream, thanks!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Did you think this day was not going to arrive ever? Your patience has been rewarded, yay, sent upstream, thanks! Closing...ciao
          Hide
          Helen Foster added a comment -

          Linking to another issue relating to the lang menu in Bootstrap/Clean.

          Show
          Helen Foster added a comment - Linking to another issue relating to the lang menu in Bootstrap/Clean.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: