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

Custom menus - sub-menus breaking in Bootstrap based themes

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. Select Clean Theme
      2. In Site Administration > Appearance > Themes > Theme setting ensure that the default menu items are added to the custom menu area, then Save settings.
      3. Test Clean theme custommenu dropdown items don't have scroll bars in x-axis (bottom left>right).
      Show
      Select Clean Theme In Site Administration > Appearance > Themes > Theme setting ensure that the default menu items are added to the custom menu area, then Save settings. Test Clean theme custommenu dropdown items don't have scroll bars in x-axis (bottom left>right).
    • Workaround:
      Hide

      Add the following CSS rule to your bootstrap theme's Custom CSS settings page.

       
      .open > .dropdown-menu {
      overflow: visible;
      }

      Show
      Add the following CSS rule to your bootstrap theme's Custom CSS settings page.   .open > .dropdown-menu { overflow: visible; }
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-41788_master_3

      Description

      When upgrading to the latest stable via GIT, it appears that someone has made alterations to the code related to the custom menu items... if I upgrade, it completely breaks the submenu functionality made available as part of the "Custom Menu Items" (made available as part of the Themes)

      NOTE that I had to revert to a previous snapshot to quickly fix the issue.

      Thanks

      Mike

        Gliffy Diagrams

        1. 2013-12-04 18_50_50-Moodle GJB.png
          23 kB
        2. MDL-41788-langmenu.jpg
          11 kB
        3. MDL-41788-submenu.jpg
          7 kB
        4. photo.PNG
          58 kB
        5. screenshot-1.jpg
          30 kB

          Issue Links

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for reporting that, Michael.

            I tried using a sub-menu in the Clean theme. It had a problem there and I've captured a screenshot. Is that what is that what you were experiencing?

            What theme are you using on your site? Is it the Essential theme?

            Show
            salvetore Michael de Raadt added a comment - Thanks for reporting that, Michael. I tried using a sub-menu in the Clean theme. It had a problem there and I've captured a screenshot. Is that what is that what you were experiencing? What theme are you using on your site? Is it the Essential theme?
            Hide
            burning-effigy Michael Otto added a comment -

            Hi Michael

            Thanks for your quick response.

            1) Yes - that is the exact problem being experienced.
            2) We are using the essential theme.

            Please let me know if there are any details I can provide

            Show
            burning-effigy Michael Otto added a comment - Hi Michael Thanks for your quick response. 1) Yes - that is the exact problem being experienced. 2) We are using the essential theme. Please let me know if there are any details I can provide
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for responding. If this is happening in both Clean and Essential then I assume it is a problem in the root Bootstrapbase theme.

            Feel free to help us find a solution.

            Show
            salvetore Michael de Raadt added a comment - Thanks for responding. If this is happening in both Clean and Essential then I assume it is a problem in the root Bootstrapbase theme. Feel free to help us find a solution.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            I can confirm it's a regression caused by MDL-40575 where I added the follwoing CSS mark-up:
             

            .open > .dropdown-menu {
                display: block;
                max-height: 500px;
                overflow-y: auto;
            }
            

            Show
            lazydaisy Mary Evans added a comment - - edited I can confirm it's a regression caused by MDL-40575 where I added the follwoing CSS mark-up:   .open > .dropdown-menu { display: block; max-height: 500px; overflow-y: auto; }
            Hide
            lazydaisy Mary Evans added a comment -

            Just adding Gareth as a watcher to see if he can suggest an alternative solution than the one I used in MDL-40575 which is now causing this regression.

            Show
            lazydaisy Mary Evans added a comment - Just adding Gareth as a watcher to see if he can suggest an alternative solution than the one I used in MDL-40575 which is now causing this regression.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Dear Mary Evans,

            Sorry, really busy at the moment, will look if I get a chance.

            G

            Show
            gb2048 Gareth J Barnard added a comment - Dear Mary Evans , Sorry, really busy at the moment, will look if I get a chance. G
            Hide
            lazydaisy Mary Evans added a comment -

            No problems ...thanks anyway

            Show
            lazydaisy Mary Evans added a comment - No problems ...thanks anyway
            Hide
            lazydaisy Mary Evans added a comment -

            Adding Andrew Nicols as a watcher.

            Show
            lazydaisy Mary Evans added a comment - Adding Andrew Nicols as a watcher.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            @Andrew, I have found a solution that will fix this but it uses jQuery but since we cannot use jQuery in Moodle (unless it is a 3rd party plugin) I was hoping you could help me convert it to JavaScript? Here is the jQuery code.
             

            var maxHeight = 400;
             
            $(function(){
             
                $(".dropdown > li").hover(function() {
                
                     var $container = $(this),
                         $list = $container.find("ul"),
                         $anchor = $container.find("a"),
                         height = $list.height() * 1.1,       // make sure there is enough room at the bottom
                         multiplier = height / maxHeight;     // needs to move faster if list is taller
                    
                    // need to save height here so it can revert on mouseout            
                    $container.data("origHeight", $container.height());
                    
                    // so it can retain it's rollover color all the while the dropdown is open
                    $anchor.addClass("hover");
                    
                    // make sure dropdown appears directly below parent list item    
                    $list
                        .show()
                        .css({
                            paddingTop: $container.data("origHeight")
                        });
                    
                    // don't do any animation if list shorter than max
                    if (multiplier > 1) {
                        $container
                            .css({
                                height: maxHeight,
                                overflow: "hidden"
                            })
                            .mousemove(function(e) {
                                var offset = $container.offset();
                                var relativeY = ((e.pageY - offset.top) * multiplier) - ($container.data("origHeight") * multiplier);
                                if (relativeY > $container.data("origHeight")) {
                                    $list.css("top", -relativeY + $container.data("origHeight"));
                                };
                            });
                    }
                    
                }, function() {
                
                    var $el = $(this);
                    
                    // put things back to normal
                    $el
                        .height($(this).data("origHeight"))
                        .find("ul")
                        .css({ top: 0 })
                        .hide()
                        .end()
                        .find("a")
                        .removeClass("hover");
                
                });
                
                // Add down arrow only to menu items with submenus
                $(".dropdown > li:has('ul')").each(function() {
                    $(this).find("a:first").append("<img src='images/down-arrow.png' />");
                });
                
            });
            
            

            Thanks in advance

            Show
            lazydaisy Mary Evans added a comment - - edited @Andrew, I have found a solution that will fix this but it uses jQuery but since we cannot use jQuery in Moodle (unless it is a 3rd party plugin) I was hoping you could help me convert it to JavaScript? Here is the jQuery code.   var maxHeight = 400 ;   $(function(){   $( ".dropdown > li" ).hover(function() { var $container = $( this ), $list = $container.find( "ul" ), $anchor = $container.find( "a" ), height = $list.height() * 1.1 , // make sure there is enough room at the bottom multiplier = height / maxHeight; // needs to move faster if list is taller // need to save height here so it can revert on mouseout $container.data( "origHeight" , $container.height()); // so it can retain it's rollover color all the while the dropdown is open $anchor.addClass( "hover" ); // make sure dropdown appears directly below parent list item $list .show() .css({ paddingTop: $container.data( "origHeight" ) }); // don't do any animation if list shorter than max if (multiplier > 1 ) { $container .css({ height: maxHeight, overflow: "hidden" }) .mousemove(function(e) { var offset = $container.offset(); var relativeY = ((e.pageY - offset.top) * multiplier) - ($container.data( "origHeight" ) * multiplier); if (relativeY > $container.data( "origHeight" )) { $list.css( "top" , -relativeY + $container.data( "origHeight" )); }; }); } }, function() { var $el = $( this ); // put things back to normal $el .height($( this ).data( "origHeight" )) .find( "ul" ) .css({ top: 0 }) .hide() .end() .find( "a" ) .removeClass( "hover" ); }); // Add down arrow only to menu items with submenus $( ".dropdown > li:has('ul')" ).each(function() { $( this ).find( "a:first" ).append( "<img src='images/down-arrow.png' />" ); }); }); Thanks in advance
            Hide
            bawjaws David Scotson added a comment -

            Hi Mary,

            are you sure that your code is causing this? Your change makes sense to me, and it's not obvious why it would cause the problem shown in the screenshot. Also I'm not seeing the problem on qa.moodle.net (though I can see your fix is working).

            Is there perhaps more to this? Can anyone point me to a publicly visible example of the problem?

            Show
            bawjaws David Scotson added a comment - Hi Mary, are you sure that your code is causing this? Your change makes sense to me, and it's not obvious why it would cause the problem shown in the screenshot. Also I'm not seeing the problem on qa.moodle.net (though I can see your fix is working). Is there perhaps more to this? Can anyone point me to a publicly visible example of the problem?
            Hide
            lazydaisy Mary Evans added a comment - - edited

            The horizontal and vertical scrolls appear when you try to select a sub-menu...there aren't any in the QA site, so you don't see the problem.
            As you have seen in the forum we are getting reports now with Long menus being truncated, but not scrolling.

            Show
            lazydaisy Mary Evans added a comment - - edited The horizontal and vertical scrolls appear when you try to select a sub-menu...there aren't any in the QA site, so you don't see the problem. As you have seen in the forum we are getting reports now with Long menus being truncated, but not scrolling.
            Hide
            lazydaisy Mary Evans added a comment -

            I am reopening this as the overflow-x: visible ; is not working as I had hoped.

            Show
            lazydaisy Mary Evans added a comment - I am reopening this as the overflow-x: visible ; is not working as I had hoped.
            Hide
            lazydaisy Mary Evans added a comment -

            @David it is a known problem in Bootstrap, and I am not sure how to fix this to work with the custommenu.

            Show
            lazydaisy Mary Evans added a comment - @David it is a known problem in Bootstrap, and I am not sure how to fix this to work with the custommenu.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            @David is the Lang menu called directly from core renderer or is there still a renderer in Bootstrapbase? If so it may be easier to add an id to that menu item and then fix only that for the longer menus. In the mean time I need to reverse MDL-40575.
            Moodle.org will need to add the MDL-40575 fix directly into their version as they do not have any subdirectory menu items.

            Show
            lazydaisy Mary Evans added a comment - - edited @David is the Lang menu called directly from core renderer or is there still a renderer in Bootstrapbase? If so it may be easier to add an id to that menu item and then fix only that for the longer menus. In the mean time I need to reverse MDL-40575 . Moodle.org will need to add the MDL-40575 fix directly into their version as they do not have any subdirectory menu items.
            Hide
            ianmcn Ian McNaught added a comment -

            Is there any quick hack/fix for this yet? Unfortunately I noticed this a week after going to 2.5.2 in production, too late to switch back! :-S

            Show
            ianmcn Ian McNaught added a comment - Is there any quick hack/fix for this yet? Unfortunately I noticed this a week after going to 2.5.2 in production, too late to switch back! :-S
            Hide
            pcrispim Pedro Crispim added a comment -

            Mary Evans posted a workaround:
            "It's a Moodle bug MDL-41788

            To fix it just add this to your Custom CSS setting within Essential theme settings.

            .open > .dropdown-menu {
            overflow: visible;
            }"

            Show
            pcrispim Pedro Crispim added a comment - Mary Evans posted a workaround: "It's a Moodle bug MDL-41788 To fix it just add this to your Custom CSS setting within Essential theme settings. .open > .dropdown-menu { overflow: visible; }"
            Hide
            lazydaisy Mary Evans added a comment -

            I have added the workaround to the area marked "Workaround" at the top of this page.
            Thanks Pedro for reminding me I needed to add this.

            Show
            lazydaisy Mary Evans added a comment - I have added the workaround to the area marked "Workaround" at the top of this page. Thanks Pedro for reminding me I needed to add this.
            Hide
            burning-effigy Michael Otto added a comment -

            Please note that I have applied the workaround to the essential theme (bootstrap) with the latest moodle code from git. it now works as expected.

            Thank you for all of your help!

            Show
            burning-effigy Michael Otto added a comment - Please note that I have applied the workaround to the essential theme (bootstrap) with the latest moodle code from git. it now works as expected. Thank you for all of your help!
            Hide
            moodle.com moodle.com added a comment -

            Removing this from the must-fix list. This is not a 2.6 specific issue. Thanks for working on this. Please continue.

            Show
            moodle.com moodle.com added a comment - Removing this from the must-fix list. This is not a 2.6 specific issue. Thanks for working on this. Please continue.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Mary Evans,

            Bumped into this whilst peer reviewing MDL-43072, and discovered that just removing:

            ....
            overflow-y: auto;
            .....

            Fixes it.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Mary Evans , Bumped into this whilst peer reviewing MDL-43072 , and discovered that just removing: .... overflow-y: auto; ..... Fixes it. Cheers, Gareth
            Hide
            lazydaisy Mary Evans added a comment -

            I know removing the the overflow-y: auto clears up this reported problem, but then the Lange menu does not scroll. Which is paramount for those who need it to scroll.
            The alternative is drop the lang menu from Bootstrapbase's top navbar. Either one of the other has to go.

            It would be easier to add the lang menu as I had it in Tiny Bootstrap.

            Show
            lazydaisy Mary Evans added a comment - I know removing the the overflow-y: auto clears up this reported problem, but then the Lange menu does not scroll. Which is paramount for those who need it to scroll. The alternative is drop the lang menu from Bootstrapbase's top navbar. Either one of the other has to go. It would be easier to add the lang menu as I had it in Tiny Bootstrap.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Fair enough Mary, there is always some other complication isn't there! Fix one thing one way and break something else = drat, drat and double drat. Please let me know if you want me to peer review the fix when you are ready.

            Show
            gb2048 Gareth J Barnard added a comment - Fair enough Mary, there is always some other complication isn't there! Fix one thing one way and break something else = drat, drat and double drat. Please let me know if you want me to peer review the fix when you are ready.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Interesting that Julian Ridden has gone for the:

            .open > .dropdown-menu {
            	overflow: visible;
            }

            solution in Essential 2.6.2.

            Show
            gb2048 Gareth J Barnard added a comment - Interesting that Julian Ridden has gone for the: .open > .dropdown-menu { overflow: visible; } solution in Essential 2.6.2.
            Hide
            lazydaisy Mary Evans added a comment -

            Gareth J Barnard I noticed that too...in that case do you want to pick this up and fix it?

            Show
            lazydaisy Mary Evans added a comment - Gareth J Barnard I noticed that too...in that case do you want to pick this up and fix it?
            Hide
            gb2048 Gareth J Barnard added a comment -

            Ok Mary Evans, will do.

            Show
            gb2048 Gareth J Barnard added a comment - Ok Mary Evans , will do.
            Hide
            trogdor Julian Ridden added a comment -

            Thanks for tagging me in this Gareth. I was not aware of this tracker item.

            Sadly I don't know if my solution is best practice, but in testing (and a lot of googling) it seemed to be the best answer I could find to the problem while waiting for it to be fixed in core.

            Julian

            Show
            trogdor Julian Ridden added a comment - Thanks for tagging me in this Gareth. I was not aware of this tracker item. Sadly I don't know if my solution is best practice, but in testing (and a lot of googling) it seemed to be the best answer I could find to the problem while waiting for it to be fixed in core. Julian
            Hide
            lazydaisy Mary Evans added a comment -

            Hi Gareth,

            Looking at this now, the real solution would be to remove the whole of the CSS which is causing the problem, and replace it with a similar CSS rule which only targets the Language menu, since this is why I added the original CSS rule.

            What I think we need is a class selector adding to the <li> tag for the language menu so that it looks like so...

            <li class="dropdown langmenu">
            <a class="dropdown-toggle" title="Language" data-toggle="dropdown" href="#">
            Language<b class="caret"></b>
            </a>

            This way we would be able to target the language menu where the CSS is needed.

            Show
            lazydaisy Mary Evans added a comment - Hi Gareth, Looking at this now, the real solution would be to remove the whole of the CSS which is causing the problem, and replace it with a similar CSS rule which only targets the Language menu, since this is why I added the original CSS rule. What I think we need is a class selector adding to the <li> tag for the language menu so that it looks like so... <li class="dropdown langmenu"> <a class="dropdown-toggle" title="Language" data-toggle="dropdown" href="#"> Language<b class="caret"></b> </a> This way we would be able to target the language menu where the CSS is needed.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Mary Evans,

            Would that mark-up change cause a regression in contributed themes?

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Mary Evans , Would that mark-up change cause a regression in contributed themes? Cheers, Gareth
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Mary Evans,

            What's the state of play please?

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Mary Evans , What's the state of play please? Cheers, Gareth
            Hide
            lazydaisy Mary Evans added a comment - - edited

            If the renderer in bootstrapbase was changed then whatever theme had bootstrapbase as a parent would inherit the change, so I would not think that would cause a regression in Contributed themes. In fact when has Moodle HQ bothered if changes they make affects contributed themes? Take Decaf as an example, the times that theme has changed because changes were made to Moodle navigation which in turn affected the Awesombar!

            Anyway whatever we do, the fix I added which is causing problems needs removing, so just remove that CSS and Moodle.org will have to add that same CSS to their official Moodle theme's custom.css file.

            So the state of play is either find a way to target the Language menu in Boostrapbase or remove the CSS.
            The choice is yours.

            Show
            lazydaisy Mary Evans added a comment - - edited If the renderer in bootstrapbase was changed then whatever theme had bootstrapbase as a parent would inherit the change, so I would not think that would cause a regression in Contributed themes. In fact when has Moodle HQ bothered if changes they make affects contributed themes? Take Decaf as an example, the times that theme has changed because changes were made to Moodle navigation which in turn affected the Awesombar! Anyway whatever we do, the fix I added which is causing problems needs removing, so just remove that CSS and Moodle.org will have to add that same CSS to their official Moodle theme's custom.css file. So the state of play is either find a way to target the Language menu in Boostrapbase or remove the CSS. The choice is yours.
            Hide
            gb2048 Gareth J Barnard added a comment - - edited

            Ok, even though it will cause more work for me. Go with removing the problematic CSS from the regressional tracker issue (MDL-40575) and I'll see if the language mark-up works. Could you finish the peer review please

            Show
            gb2048 Gareth J Barnard added a comment - - edited Ok, even though it will cause more work for me. Go with removing the problematic CSS from the regressional tracker issue ( MDL-40575 ) and I'll see if the language mark-up works. Could you finish the peer review please
            Hide
            gb2048 Gareth J Barnard added a comment -

            Ok, looking at the 'add class to menu' option, far more difficult as the class 'custom_menu_item' in '/lib/outputcomponents.php' does not have an attribute for associated classes. And therefore when rendered in 'render_custom_menu_item' after it has been set up by 'render_custom_menu' in '/theme/bootstrapbase/renderers/core_renderer.php'.

            Show
            gb2048 Gareth J Barnard added a comment - Ok, looking at the 'add class to menu' option, far more difficult as the class 'custom_menu_item' in '/lib/outputcomponents.php' does not have an attribute for associated classes. And therefore when rendered in 'render_custom_menu_item' after it has been set up by 'render_custom_menu' in '/theme/bootstrapbase/renderers/core_renderer.php'.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Will look into a bootstrap base option only.

            Show
            gb2048 Gareth J Barnard added a comment - Will look into a bootstrap base option only.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Ok, created https://github.com/gjb2048/moodle/compare/moodle:MOODLE_25_STABLE...wip-MDL-41788_M25_2 but bashing my head against the wall and even though the CSS is applied it does not solve the problem. Perhaps the other solution is better as targets all menu's. Need to get on with other things.

            Show
            gb2048 Gareth J Barnard added a comment - Ok, created https://github.com/gjb2048/moodle/compare/moodle:MOODLE_25_STABLE...wip-MDL-41788_M25_2 but bashing my head against the wall and even though the CSS is applied it does not solve the problem. Perhaps the other solution is better as targets all menu's. Need to get on with other things.
            Hide
            bawjaws David Scotson added a comment -

            If you know that a language menu longer than the screen is going to be used on your site (which I'm assuming is rare) then you should be able to target it via CSS, since it'll always be the last one in the row.

            So we could revert the CSS for fixing long menus and then let sites that have very long lang menus add it back in their own theme CSS.

            They could just use the same CSS (if they have a long menu, but no sub-menus):

             
            .open > .dropdown-menu {
                display: block;
                max-height: 500px;
                overflow-y: auto;
            }

            or limit it to the last menu item if they need both a long menu and a sub-menu (not actually tested this, but should work in browsers after IE8):

            ul.nav .open:last-child > .dropdown-menu {
                display: block;
                max-height: 500px;
                overflow-y: auto;
            }

            This is based on the assumption that sites with menu lists longer than the screen are fewer than those with sub-menus. Personally, I'd advise strongly against having either if you can avoid them, but it's my impression that sub-menus are the more popular of the two and hence the bigger bug.

            I'm sure there's all sorts of other cleverness you could do based on checking if there's more than 20 or so languages installed and doing something else in that case, but is this actually a problem for anyone other sites demonstrating Moodle's impressive range of localization?

            Show
            bawjaws David Scotson added a comment - If you know that a language menu longer than the screen is going to be used on your site (which I'm assuming is rare) then you should be able to target it via CSS, since it'll always be the last one in the row. So we could revert the CSS for fixing long menus and then let sites that have very long lang menus add it back in their own theme CSS. They could just use the same CSS (if they have a long menu, but no sub-menus): .open > .dropdown-menu { display: block; max-height: 500px; overflow-y: auto; } or limit it to the last menu item if they need both a long menu and a sub-menu (not actually tested this, but should work in browsers after IE8): ul.nav .open:last-child > .dropdown-menu { display: block; max-height: 500px; overflow-y: auto; } This is based on the assumption that sites with menu lists longer than the screen are fewer than those with sub-menus. Personally, I'd advise strongly against having either if you can avoid them, but it's my impression that sub-menus are the more popular of the two and hence the bigger bug. I'm sure there's all sorts of other cleverness you could do based on checking if there's more than 20 or so languages installed and doing something else in that case, but is this actually a problem for anyone other sites demonstrating Moodle's impressive range of localization?
            Hide
            gb2048 Gareth J Barnard added a comment -

            So... Does the current solution as it stands fix the custom menu issue as well as the language scrolling issue?

            Show
            gb2048 Gareth J Barnard added a comment - So... Does the current solution as it stands fix the custom menu issue as well as the language scrolling issue?
            Hide
            lazydaisy Mary Evans added a comment - - edited

            In a nutshell NO!
            Sorry, retracting what I said earlier.
            I did not see that you had added a new branch I thought you meant the one in the top of this page.
            I'm just going to check it out.

            Show
            lazydaisy Mary Evans added a comment - - edited In a nutshell NO! Sorry, retracting what I said earlier. I did not see that you had added a new branch I thought you meant the one in the top of this page. I'm just going to check it out.
            Hide
            lazydaisy Mary Evans added a comment -

            Finished Peer Review

            Show
            lazydaisy Mary Evans added a comment - Finished Peer Review
            Hide
            lazydaisy Mary Evans added a comment -

            Sorry Gareth...I had to go out and so was AFK when you asked to end peer review.

            Show
            lazydaisy Mary Evans added a comment - Sorry Gareth...I had to go out and so was AFK when you asked to end peer review.
            Hide
            gb2048 Gareth J Barnard added a comment -

            So in essence, the https://github.com/gjb2048/moodle/compare/moodle:MOODLE_25_STABLE...wip-MDL-41788_M25_2 solution is in theory the right one even though the CSS is being applied but does not work! = Help as busy elsewhere.

            Show
            gb2048 Gareth J Barnard added a comment - So in essence, the https://github.com/gjb2048/moodle/compare/moodle:MOODLE_25_STABLE...wip-MDL-41788_M25_2 solution is in theory the right one even though the CSS is being applied but does not work! = Help as busy elsewhere.
            Hide
            lazydaisy Mary Evans added a comment -

            No I missed reading your other comment I'm just testing this now!!!

            Show
            lazydaisy Mary Evans added a comment - No I missed reading your other comment I'm just testing this now!!!
            Hide
            lazydaisy Mary Evans added a comment -

            Gareth, is your latest branch based on MOODLE_25_STABLE as I am getting all kinds of files loded that just do not tally with Moodle 2.5 they look like 1.9 themes?

            Show
            lazydaisy Mary Evans added a comment - Gareth, is your latest branch based on MOODLE_25_STABLE as I am getting all kinds of files loded that just do not tally with Moodle 2.5 they look like 1.9 themes?
            Hide
            gb2048 Gareth J Barnard added a comment -

            Branch 'wip-MDL-41788_M25_2' is based on 'MOODLE_25_STABLE' as you can see from the comparison: https://github.com/gjb2048/moodle/compare/moodle:MOODLE_25_STABLE...wip-MDL-41788_M25_2.

            Show
            gb2048 Gareth J Barnard added a comment - Branch 'wip- MDL-41788 _M25_2' is based on 'MOODLE_25_STABLE' as you can see from the comparison: https://github.com/gjb2048/moodle/compare/moodle:MOODLE_25_STABLE...wip-MDL-41788_M25_2 .
            Hide
            lazydaisy Mary Evans added a comment -

            sorry my fault

            Show
            lazydaisy Mary Evans added a comment - sorry my fault
            Hide
            gb2048 Gareth J Barnard added a comment -

            No worries

            Show
            gb2048 Gareth J Barnard added a comment - No worries
            Hide
            lazydaisy Mary Evans added a comment -

            Gareth...wip-MDL-41788_M25_2 works!!!
            It's brilliant!

            Show
            lazydaisy Mary Evans added a comment - Gareth...wip- MDL-41788 _M25_2 works!!! It's brilliant!
            Hide
            gb2048 Gareth J Barnard added a comment -

            Really! ? I could not get the CSS to operate in Chrome with around 15 languages. I hope the PHP is ok as I've kept the changes confined to Bootstrapbase only and away from the rest of the API.

            Show
            gb2048 Gareth J Barnard added a comment - Really! ? I could not get the CSS to operate in Chrome with around 15 languages. I hope the PHP is ok as I've kept the changes confined to Bootstrapbase only and away from the rest of the API.
            Hide
            lazydaisy Mary Evans added a comment -

            Just uploading images...will do more testing after tea

            Show
            lazydaisy Mary Evans added a comment - Just uploading images...will do more testing after tea
            Hide
            lazydaisy Mary Evans added a comment -

            Adding MDL-41788-submenu.jpg and MDL-41788-langmenu.jpg

            Show
            lazydaisy Mary Evans added a comment - Adding MDL-41788 -submenu.jpg and MDL-41788 -langmenu.jpg
            Hide
            gb2048 Gareth J Barnard added a comment -

            Ok - If all is well, I'll do the master branch tomorrow as need a bit of a break!

            Show
            gb2048 Gareth J Barnard added a comment - Ok - If all is well, I'll do the master branch tomorrow as need a bit of a break!
            Hide
            lazydaisy Mary Evans added a comment -

            Works OK in Chrome too...

            I love the way you did the renderer. It was just what I was thinking but did not have a clue how to do it.

            Show
            lazydaisy Mary Evans added a comment - Works OK in Chrome too... I love the way you did the renderer. It was just what I was thinking but did not have a clue how to do it.
            Hide
            lazydaisy Mary Evans added a comment -

            Might as well leave it until master is updated same for M25 branch as they will only lead to conflicts with rebase otherwise.
            Thanks for all the hard work.

            Show
            lazydaisy Mary Evans added a comment - Might as well leave it until master is updated same for M25 branch as they will only lead to conflicts with rebase otherwise. Thanks for all the hard work.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Mary Evans,

            No worries. I've just altered the rendering code to be a bit more pretty, so rebased and squashed. The thing is in Chrome I can't get the language menu to put show a scroll bar . Pic to follow.

            G

            Show
            gb2048 Gareth J Barnard added a comment - Hi Mary Evans , No worries. I've just altered the rendering code to be a bit more pretty, so rebased and squashed. The thing is in Chrome I can't get the language menu to put show a scroll bar . Pic to follow. G
            Show
            gb2048 Gareth J Barnard added a comment - https://tracker.moodle.org/secure/attachment/36037/2013-12-04%2018_50_50-Moodle%20GJB.png Added.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Mary Evans,

            I've gone for:

            .langmenu.open > .dropdown-menu {
                display: block;
                max-height: 150px;
                overflow-y: auto;
            }

            Which works well on small devices 'photo.PNG' and net books of 1024 x 600.

            M2.5 branch updated and rebased. M2.6 / master to follow tomorrow.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Mary Evans , I've gone for: .langmenu.open > .dropdown-menu { display: block; max-height: 150px; overflow-y: auto; } Which works well on small devices 'photo.PNG' and net books of 1024 x 600. M2.5 branch updated and rebased. M2.6 / master to follow tomorrow. Cheers, Gareth
            Hide
            lazydaisy Mary Evans added a comment - - edited

            That's OK thanks.
            By the way don't forget to update the branches listed at the top of this page,
            otherwise the wrong patch will get integrated.

            Show
            lazydaisy Mary Evans added a comment - - edited That's OK thanks. By the way don't forget to update the branches listed at the top of this page, otherwise the wrong patch will get integrated.
            Hide
            lazydaisy Mary Evans added a comment -

            Hold your horses Gareth,,,please look at MDL-40771 before you proceed as there is a greater possibility it will cause conflicts with your patch here.

            Show
            lazydaisy Mary Evans added a comment - Hold your horses Gareth,,,please look at MDL-40771 before you proceed as there is a greater possibility it will cause conflicts with your patch here.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            Fortunately Gareth MDL-40771 is only affecting master branch as far as I can make out. So for your patch here to be integrated in the next PULL you could leave off master branch until Sam's patch has been integrated. Or base Master branch on Sam's in a new tracker?

            Show
            lazydaisy Mary Evans added a comment - - edited Fortunately Gareth MDL-40771 is only affecting master branch as far as I can make out. So for your patch here to be integrated in the next PULL you could leave off master branch until Sam's patch has been integrated. Or base Master branch on Sam's in a new tracker?
            Hide
            gb2048 Gareth J Barnard added a comment -

            Oh fudge!

            Looking at the changes now to see what I need to do. I'd rather wait until MDL-40771 lands and see if it's back-ported and then base my code on that.

            Show
            gb2048 Gareth J Barnard added a comment - Oh fudge! Looking at the changes now to see what I need to do. I'd rather wait until MDL-40771 lands and see if it's back-ported and then base my code on that.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Ok, looking at https://github.com/samhemelryk/moodle/compare/master...wip-MDL-40771-m26#diff-1ebda6b6acc9f8ef4496ae12a5f88f84R2739 there is a new class 'custom_menu_language_item' which extends 'custom_menu_item' so therefore in https://github.com/samhemelryk/moodle/blob/252f63198d26454a898ac90da946c899447091ea/theme/bootstrapbase/renderers/core_renderer.php#L91 it might be possible to detect that the $menunode custom_menu_item instance is in fact an instance of custom_menu_language_item and if so add the class as per the second M2.5 solution.

            Show
            gb2048 Gareth J Barnard added a comment - Ok, looking at https://github.com/samhemelryk/moodle/compare/master...wip-MDL-40771-m26#diff-1ebda6b6acc9f8ef4496ae12a5f88f84R2739 there is a new class 'custom_menu_language_item' which extends 'custom_menu_item' so therefore in https://github.com/samhemelryk/moodle/blob/252f63198d26454a898ac90da946c899447091ea/theme/bootstrapbase/renderers/core_renderer.php#L91 it might be possible to detect that the $menunode custom_menu_item instance is in fact an instance of custom_menu_language_item and if so add the class as per the second M2.5 solution.
            Hide
            lazydaisy Mary Evans added a comment -

            Show
            lazydaisy Mary Evans added a comment -
            Hide
            gb2048 Gareth J Barnard added a comment -

            Bingo! PHP has 'instanceof' -> http://www.php.net/manual/en/internals2.opcodes.instanceof.php so should be viable.

            Show
            gb2048 Gareth J Barnard added a comment - Bingo! PHP has 'instanceof' -> http://www.php.net/manual/en/internals2.opcodes.instanceof.php so should be viable.
            Hide
            lazydaisy Mary Evans added a comment -

            What are you going to do then? Just go with fixing 2.5 and 2.6 and basing your fix for master on Sams branch? or wait another week or more as Sam's patch may get the thumbs down if Damyon s about. LOL

            Show
            lazydaisy Mary Evans added a comment - What are you going to do then? Just go with fixing 2.5 and 2.6 and basing your fix for master on Sams branch? or wait another week or more as Sam's patch may get the thumbs down if Damyon s about. LOL
            Hide
            gb2048 Gareth J Barnard added a comment -

            I'm going to see what happens to MDL-40771 as it's further along than this and has an impact on what the solution for this will be.

            Show
            gb2048 Gareth J Barnard added a comment - I'm going to see what happens to MDL-40771 as it's further along than this and has an impact on what the solution for this will be.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Waiting for further developments on MDL-40771.

            Show
            gb2048 Gareth J Barnard added a comment - Waiting for further developments on MDL-40771 .
            Hide
            lazydaisy Mary Evans added a comment -

            Hi Gareth,
            Don't wait for Sam's patch as it is not going anywhere. It would be better that we get this fixed before MDL-40771 then Sam will have to work around it.

            You can also take Blocker off this now.

            Thanks
            Mary

            Show
            lazydaisy Mary Evans added a comment - Hi Gareth, Don't wait for Sam's patch as it is not going anywhere. It would be better that we get this fixed before MDL-40771 then Sam will have to work around it. You can also take Blocker off this now. Thanks Mary
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Mary Evans,

            All branches now done. Requesting peer review.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Mary Evans , All branches now done. Requesting peer review. Cheers, Gareth
            Hide
            lazydaisy Mary Evans added a comment -

            Looks OK and tested previously so submitting this for Integration Review.

            Show
            lazydaisy Mary Evans added a comment - Looks OK and tested previously so submitting this for Integration Review.
            Hide
            marina Marina Glancy added a comment -

            Hi, thanks for working on this Gareth.
            Unfortunately we can not add parameters to the methods in stable versions, it will break all themes that have already overridden this particular method in their renderers.

            What we could do is to add some additional attribute to the custom_menu_item class. But still you can not change constructor arguments, so new method is needed to set the value for this attribute.

            Would also be really nice to hear some feedback from Sam Hemelryk because of it's similarity to MDL-40771

            Show
            marina Marina Glancy added a comment - Hi, thanks for working on this Gareth. Unfortunately we can not add parameters to the methods in stable versions, it will break all themes that have already overridden this particular method in their renderers. What we could do is to add some additional attribute to the custom_menu_item class. But still you can not change constructor arguments, so new method is needed to set the value for this attribute. Would also be really nice to hear some feedback from Sam Hemelryk because of it's similarity to MDL-40771
            Hide
            gb2048 Gareth J Barnard added a comment - - edited

            Thanks Marina Glancy, oh that is so frustrating even with an optional parameter. Therefore I suppose I only have two alternatives:

            1. Create a member variable and set so that can be used by other methods.
            2. Create the child class custom_language_menu_item of custom_menu_item as in MDL-40771 and use 'instanceof' in render_custom_menu_item to determine whether to add in the extra 'langmenu' class.

            Both of which seem to be overkill just to add a CSS class to the rendered output.

            But in future I think custom_menu_item should contain an attribute containing an array of CSS class names to be used when it is rendered. Its just a bit too restrictive at the moment.

            Show
            gb2048 Gareth J Barnard added a comment - - edited Thanks Marina Glancy , oh that is so frustrating even with an optional parameter. Therefore I suppose I only have two alternatives: 1. Create a member variable and set so that can be used by other methods. 2. Create the child class custom_language_menu_item of custom_menu_item as in MDL-40771 and use 'instanceof' in render_custom_menu_item to determine whether to add in the extra 'langmenu' class. Both of which seem to be overkill just to add a CSS class to the rendered output. But in future I think custom_menu_item should contain an attribute containing an array of CSS class names to be used when it is rendered. Its just a bit too restrictive at the moment.
            Hide
            gb2048 Gareth J Barnard added a comment -

            A question. In PHP can the method in an object access the attributes of another object if they are the same class or derived from the same class? I.e. when the custom_language_menu_item is created it could be possible to pass in the parent reference to the custom_menu being a custom_menu_item and then can I add the custom_language_menu_item using 'this' to the array of children being an attribute of custom_menu_item but on the custom_menu object? I think in Java you can because it is said that methods in the same class should know what they are doing and if you have references to both objects then you can bypass methods and access member attributes directly.

            Show
            gb2048 Gareth J Barnard added a comment - A question. In PHP can the method in an object access the attributes of another object if they are the same class or derived from the same class? I.e. when the custom_language_menu_item is created it could be possible to pass in the parent reference to the custom_menu being a custom_menu_item and then can I add the custom_language_menu_item using 'this' to the array of children being an attribute of custom_menu_item but on the custom_menu object? I think in Java you can because it is said that methods in the same class should know what they are doing and if you have references to both objects then you can bypass methods and access member attributes directly.
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            lazydaisy Mary Evans added a comment - - edited

            In that case we need to take out the offending CSS that breaks the menu.
            The sooner they remove the Lang menu from the main menu in Bootstrapbase the better...then we can move forward, as this is becoming ridiculous.

            Show
            lazydaisy Mary Evans added a comment - - edited In that case we need to take out the offending CSS that breaks the menu. The sooner they remove the Lang menu from the main menu in Bootstrapbase the better...then we can move forward, as this is becoming ridiculous.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Ok, I've slept on it and as this is pressing, see: https://moodle.org/mod/forum/discuss.php?d=245657 - I'm going for the localised class attribute solution '1' above (being "Create a member variable and set so that can be used by other methods.") that will encapsulate within Bootstrapbase only and not affect anything else and additionally be specific to the language menu.

            If MDL-40771 etc. wants a whole new class etc. then it can do so in the future.

            Show
            gb2048 Gareth J Barnard added a comment - Ok, I've slept on it and as this is pressing, see: https://moodle.org/mod/forum/discuss.php?d=245657 - I'm going for the localised class attribute solution '1' above (being "Create a member variable and set so that can be used by other methods.") that will encapsulate within Bootstrapbase only and not affect anything else and additionally be specific to the language menu. If MDL-40771 etc. wants a whole new class etc. then it can do so in the future.
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hi Mary Evans,

            Revised localised solution implemented, all main branches rebased to latest versions, new branches created from them (being from MOODLE_25_STABLE, MOODLE_26_STABLE and master). Please could you peer review.

            Cheers,

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hi Mary Evans , Revised localised solution implemented, all main branches rebased to latest versions, new branches created from them (being from MOODLE_25_STABLE, MOODLE_26_STABLE and master). Please could you peer review. Cheers, Gareth
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Hi guys,

            this is just a message to share with you that I'm going to perform a test of the automated pre-checker against all the current issues awaiting integration (16 issues).

            So, soon, you'll get some extra comments in this issue with some information from the pre-checker. Note it's not final, but just an experiment and there are lots of things to improve, from the message itself to various false positives in the checkers. So take any report with caution, it's not 100% accurate yet.

            Please, feel free to comment any idea/objection @ MDLSITE-2662. I'll be collecting everything there.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Hi guys, this is just a message to share with you that I'm going to perform a test of the automated pre-checker against all the current issues awaiting integration (16 issues). So, soon, you'll get some extra comments in this issue with some information from the pre-checker. Note it's not final, but just an experiment and there are lots of things to improve, from the message itself to various false positives in the checkers. So take any report with caution, it's not 100% accurate yet. Please, feel free to comment any idea/objection @ MDLSITE-2662 . I'll be collecting everything there. TIA and ciao
            Hide
            cibot CiBoT added a comment -

            Results for MDL-41788

            Show
            cibot CiBoT added a comment - Results for MDL-41788 Branch wip- MDL-41788 _M25_3 to be integrated into upstream MOODLE_25_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/676 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/676/artifact/work/smurf.html Branch wip- MDL-41788 _M26_3 to be integrated into upstream MOODLE_26_STABLE Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/677 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/677/artifact/work/smurf.html Branch wip- MDL-41788 _master_3 to be integrated into upstream master Executed job http://ci.stronk7.com/job/Precheck%20remote%20branch/678 Execution status: 0 Details: http://ci.stronk7.com/job/Precheck%20remote%20branch/678/artifact/work/smurf.html
            Hide
            lazydaisy Mary Evans added a comment -

            Gareth J Barnard Get thee from thy TV chair => never mind the Robbers Tale there's trouble at mill!

            Show
            lazydaisy Mary Evans added a comment - Gareth J Barnard Get thee from thy TV chair => never mind the Robbers Tale there's trouble at mill!
            Hide
            gb2048 Gareth J Barnard added a comment -

            Aye Mary Evans, ny worry lass, thy be thee nay trouble down tut mill .

            Line 140 not having the space before the '=>' as reported by: http://ci.stronk7.com/job/Precheck%20remote%20branch/678/artifact/work/smurf.html - is not the fault of this patch as it is an existing issue as the pre-patch code has the fault: https://github.com/moodle/moodle/blob/master/theme/bootstrapbase/renderers/core_renderer.php#L134 - and therefore this patch cannot be blamed for introducing any new issues. As seen on:https://github.com/gjb2048/moodle/blob/85e88c7b4f6d4c0a17dc91c1cc62d22cc362140d/theme/bootstrapbase/renderers/core_renderer.php.

            I have been told repeatedly not to clean up existing code as it will not get through. Only if it is in the direct area. I think in this case to delay a major patch because of an existing missing space would be very frustrating and disheartening. I'm not in a position to fix this evening but if nothing is done then will get a chance tomorrow.

            Eee by gum!

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Aye Mary Evans , ny worry lass, thy be thee nay trouble down tut mill . Line 140 not having the space before the '=>' as reported by: http://ci.stronk7.com/job/Precheck%20remote%20branch/678/artifact/work/smurf.html - is not the fault of this patch as it is an existing issue as the pre-patch code has the fault: https://github.com/moodle/moodle/blob/master/theme/bootstrapbase/renderers/core_renderer.php#L134 - and therefore this patch cannot be blamed for introducing any new issues. As seen on: https://github.com/gjb2048/moodle/blob/85e88c7b4f6d4c0a17dc91c1cc62d22cc362140d/theme/bootstrapbase/renderers/core_renderer.php . I have been told repeatedly not to clean up existing code as it will not get through. Only if it is in the direct area. I think in this case to delay a major patch because of an existing missing space would be very frustrating and disheartening. I'm not in a position to fix this evening but if nothing is done then will get a chance tomorrow. Eee by gum! Gareth
            Hide
            gb2048 Gareth J Barnard added a comment - - edited

            And looking at the rest of the code it would be a case of pot, kettle and black. => http://en.wikipedia.org/wiki/The_pot_calling_the_kettle_black

            Show
            gb2048 Gareth J Barnard added a comment - - edited And looking at the rest of the code it would be a case of pot, kettle and black. => http://en.wikipedia.org/wiki/The_pot_calling_the_kettle_black
            Hide
            lazydaisy Mary Evans added a comment -

            LOL

            Show
            lazydaisy Mary Evans added a comment - LOL
            Hide
            gb2048 Gareth J Barnard added a comment -

            Hey up flower, tut mill trouble has been thee sorted lass.

            I've corrected the space before and after issue on line 140 as per the coding standard, rebased and squashed all branches. Code identical in functionality to that peer reviewed but will take up an extra two bytes (possibly four if UTF-8) of space on the server.

            I'm off to walk thee whippets.

            Gareth

            Show
            gb2048 Gareth J Barnard added a comment - Hey up flower, tut mill trouble has been thee sorted lass. I've corrected the space before and after issue on line 140 as per the coding standard, rebased and squashed all branches. Code identical in functionality to that peer reviewed but will take up an extra two bytes (possibly four if UTF-8) of space on the server. I'm off to walk thee whippets. Gareth
            Hide
            lazydaisy Mary Evans added a comment -

            Show
            lazydaisy Mary Evans added a comment -
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys this has been integrated now.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Tested during integration review and passed

            Show
            samhemelryk Sam Hemelryk added a comment - Tested during integration review and passed
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thank you, your code has landed just in time for 2013.
            Merry Christmas and may your 2014 be even better than 2013.

            Kind regards with much holiday spirit
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thank you, your code has landed just in time for 2013. Merry Christmas and may your 2014 be even better than 2013. Kind regards with much holiday spirit Sam
            Hide
            gb2048 Gareth J Barnard added a comment -

            Thank you Sam Hemelryk, have a wonderful Christmas and a superb new year too .

            Show
            gb2048 Gareth J Barnard added a comment - Thank you Sam Hemelryk , have a wonderful Christmas and a superb new year too .
            Hide
            lazydaisy Mary Evans added a comment -

            Thanks for saving the day with this Gareth. Thanks also to Sam for getting this integrated, tested and passed.

            Have a Happy Christmas both of you.

            Show
            lazydaisy Mary Evans added a comment - Thanks for saving the day with this Gareth. Thanks also to Sam for getting this integrated, tested and passed. Have a Happy Christmas both of you.

              People

              • Votes:
                6 Vote for this issue
                Watchers:
                16 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/May/13