Details

    • Type: Sub-task
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.3
    • Component/s: Themes
    • Testing Instructions:
      Hide

      This adds custom menu support to themes Anomaly and Serenity. To test, choose Anomaly and Serenity as themes and confirm that the custom menu appears and works.

      Show
      This adds custom menu support to themes Anomaly and Serenity. To test, choose Anomaly and Serenity as themes and confirm that the custom menu appears and works.
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Someoe pointed out that custommenu wasn't implemented in many core themes yet

      http://moodle.org/mod/forum/discuss.php?d=153741

      What are the plans with this?

        Gliffy Diagrams

          Issue Links

            Activity

            dougiamas Martin Dougiamas created issue -
            Hide
            ptrkmkl Patrick Malley added a comment -

            Martin - I think the idea is great. Unfortunately, the implementation is far from elegant as it requires overwriting the sam skin styles and loading CSS before the javascript. I spent a few hours working with the code when Sam first announced it but gave up to add this: MDL-22450.

            The bottom line is that I have too many other things on my plate right now to figure out all the bugs I was getting while trying to modify the menu. If this is something you want to see in some of my future themes (as I do), perhaps Sam can add a dropdown to formfactor. Since the menu is his creation, this will give me something of a template for future themes.

            BTW - the reason that the Standard theme doesn't provide much of a template is because it uses the default sam skin. I need an example of a theme that strips that away to leave just a simple bar.

            The perfect situation is to not require any pre-javascript CSS and to load only the structural CSS for the dropdown into the theme. It seems strange that we did so much to give themes freedom of design choice only to add this menu that loads a specific style.

            Show
            ptrkmkl Patrick Malley added a comment - Martin - I think the idea is great. Unfortunately, the implementation is far from elegant as it requires overwriting the sam skin styles and loading CSS before the javascript. I spent a few hours working with the code when Sam first announced it but gave up to add this: MDL-22450 . The bottom line is that I have too many other things on my plate right now to figure out all the bugs I was getting while trying to modify the menu. If this is something you want to see in some of my future themes (as I do), perhaps Sam can add a dropdown to formfactor. Since the menu is his creation, this will give me something of a template for future themes. BTW - the reason that the Standard theme doesn't provide much of a template is because it uses the default sam skin. I need an example of a theme that strips that away to leave just a simple bar. The perfect situation is to not require any pre-javascript CSS and to load only the structural CSS for the dropdown into the theme. It seems strange that we did so much to give themes freedom of design choice only to add this menu that loads a specific style.
            Hide
            dougiamas Martin Dougiamas added a comment -

            I seem to remember some cross browser problems, that might explain some of the design choices.

            Thanks for the info, I'll ask Sam!

            Show
            dougiamas Martin Dougiamas added a comment - I seem to remember some cross browser problems, that might explain some of the design choices. Thanks for the info, I'll ask Sam!
            Hide
            ptrkmkl Patrick Malley added a comment -

            I know is that I use the YUI dropdown menu in all of my themes that require a menu. It looks great in all browsers and requires no unusual CSS - for example - http://moodle.org/mod/data/view.php?d=26&rid=3555

            Show
            ptrkmkl Patrick Malley added a comment - I know is that I use the YUI dropdown menu in all of my themes that require a menu. It looks great in all browsers and requires no unusual CSS - for example - http://moodle.org/mod/data/view.php?d=26&rid=3555
            Hide
            dougiamas Martin Dougiamas added a comment -

            When I visit http://www.newschoollearning.com/moodle/?&theme=afterburner with JS off then the menu is dead...

            Show
            dougiamas Martin Dougiamas added a comment - When I visit http://www.newschoollearning.com/moodle/?&theme=afterburner with JS off then the menu is dead...
            Hide
            ptrkmkl Patrick Malley added a comment -

            The Afterburner theme degrades with javascript turned off. The dropdown icons disappear and the top-level links still work. As long as the admin sets those top level links to an index or other destination, a user with javascript disabled will still be able to navigate the site. But that misses my point. Moodle 2 uses a newer version of YUI. If that newer version (and Sam's trickery) allows for more elegant degradation than my Afterburner, then that's great. Why can't we come up with a better way to pull out the structural elements of the YUI menu instead of the entire sam skin?

            In other words, instead of loading the menu CSS from /node-menunav/assets/skins/sam/ - why can't we just load the core CSS from /node-menunav/assets/?

            Show
            ptrkmkl Patrick Malley added a comment - The Afterburner theme degrades with javascript turned off. The dropdown icons disappear and the top-level links still work. As long as the admin sets those top level links to an index or other destination, a user with javascript disabled will still be able to navigate the site. But that misses my point. Moodle 2 uses a newer version of YUI. If that newer version (and Sam's trickery) allows for more elegant degradation than my Afterburner, then that's great. Why can't we come up with a better way to pull out the structural elements of the YUI menu instead of the entire sam skin? In other words, instead of loading the menu CSS from /node-menunav/assets/skins/sam/ - why can't we just load the core CSS from /node-menunav/assets/?
            Hide
            dougiamas Martin Dougiamas added a comment -

            OK, cool, thanks Patrick. I really don't know any details of this one.

            Sam, do you have some spare cycles to look at this?

            Show
            dougiamas Martin Dougiamas added a comment - OK, cool, thanks Patrick. I really don't know any details of this one. Sam, do you have some spare cycles to look at this?
            dougiamas Martin Dougiamas made changes -
            Field Original Value New Value
            Assignee Patrick Malley [ ptrkmkl ] Sam Hemelryk [ samhemelryk ]
            dougiamas Martin Dougiamas made changes -
            Priority Minor [ 4 ] Major [ 3 ]
            Hide
            ptrkmkl Patrick Malley added a comment -

            For what it's worth, a lot of the contest submissions I'm getting are using the menu. However, to further my case, most of them are using the defaults set by Sam and the sam skin in yui.

            Show
            ptrkmkl Patrick Malley added a comment - For what it's worth, a lot of the contest submissions I'm getting are using the menu. However, to further my case, most of them are using the defaults set by Sam and the sam skin in yui.
            Hide
            samhemelryk Sam Hemelryk added a comment - - edited

            Hi guys,

            First up I don't know a whole lot about the inner workings of the menunav component, there is a lot of functionality built into it and I believe set methods of degradation, Moodle's integration with it is pretty basic, there is a single initialisation method in javascript-static to kick off the module, and the initial structure is produced by the core renderer if I remember correctly. Unfortunately there isn't much magic in it and thus no easy solution to this.

            In regards to the skin, the problem faced here is that the YUI loader automatically loads the requirements for the menunav component, which in this case include the skin files. Unfortunately there is not much we can do about that, however there are solutions to this problem.

            Restructure the use of the yui3-sam-skin class.
            This would be a core change, the YUI sam skin is only put into use if the components wrapping container or the body have the `yui3-skin-sam` class. Within Moodle this class is added to the body of every page, leading every YUI component to be skinned by default.
            Obviously the solution is to remove that class from the body and instead require any use of components that wish to be skinned to ensure they wrap the components content in a div with that class.
            Deciding to go this way would require that we review all YUI component usages and if required modify to either print the required node in PHP or inject the node prior to component's initialisation.
            The other thing to decide upon would be whether we some how give the theme a chance to include the skin or not... I'm not sure how we would go, would it be a one off thing, or per component??

            Alternatively we could use another menu.
            Another core change, we could of course move away from the YUI3 menunav component, and create a custom drop down menu. It wouldn't be as flexible presumably, but would have the advantage that we could include the core CSS in base, and the style CSS in standard, then theme's could copy the style CSS if they desire to use it.
            I don't like this solution one bit, but it is a solution.

            Stick with what we've got and write a doc on overwriting the menunav component.
            I've got a mountain of stuff on my plate at the moment but I believe I could whip up a doc in a couple of hours if required. The goal of the doc would simply be to look at how to destyle the custom menu and then how to build it up again. If there is a certain theme + look you would like me to do for the doc I can, even better if there is some existing CSS.

            And finally I'm open to suggestions, if any possible solutions spring to mind let me know

            My personal preference: Restructure the use of the yui3-sam-skin class is ideal if we had time to come up with a solution that worked in all cases and didn't overcomplicate the matter, however writing the doc may be a more immediate solution. Not that the first solution could potentially introduce and API change so we need to get this sorted ASAP.
            That being said perhaps I'm overlooking the obvious in someway so please you two let me know if you have any ideas!

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - - edited Hi guys, First up I don't know a whole lot about the inner workings of the menunav component, there is a lot of functionality built into it and I believe set methods of degradation, Moodle's integration with it is pretty basic, there is a single initialisation method in javascript-static to kick off the module, and the initial structure is produced by the core renderer if I remember correctly. Unfortunately there isn't much magic in it and thus no easy solution to this. In regards to the skin, the problem faced here is that the YUI loader automatically loads the requirements for the menunav component, which in this case include the skin files. Unfortunately there is not much we can do about that, however there are solutions to this problem. Restructure the use of the yui3-sam-skin class. This would be a core change, the YUI sam skin is only put into use if the components wrapping container or the body have the `yui3-skin-sam` class. Within Moodle this class is added to the body of every page, leading every YUI component to be skinned by default. Obviously the solution is to remove that class from the body and instead require any use of components that wish to be skinned to ensure they wrap the components content in a div with that class. Deciding to go this way would require that we review all YUI component usages and if required modify to either print the required node in PHP or inject the node prior to component's initialisation. The other thing to decide upon would be whether we some how give the theme a chance to include the skin or not... I'm not sure how we would go, would it be a one off thing, or per component?? Alternatively we could use another menu. Another core change, we could of course move away from the YUI3 menunav component, and create a custom drop down menu. It wouldn't be as flexible presumably, but would have the advantage that we could include the core CSS in base, and the style CSS in standard, then theme's could copy the style CSS if they desire to use it. I don't like this solution one bit, but it is a solution. Stick with what we've got and write a doc on overwriting the menunav component. I've got a mountain of stuff on my plate at the moment but I believe I could whip up a doc in a couple of hours if required. The goal of the doc would simply be to look at how to destyle the custom menu and then how to build it up again. If there is a certain theme + look you would like me to do for the doc I can, even better if there is some existing CSS. And finally I'm open to suggestions, if any possible solutions spring to mind let me know My personal preference: Restructure the use of the yui3-sam-skin class is ideal if we had time to come up with a solution that worked in all cases and didn't overcomplicate the matter, however writing the doc may be a more immediate solution. Not that the first solution could potentially introduce and API change so we need to get this sorted ASAP. That being said perhaps I'm overlooking the obvious in someway so please you two let me know if you have any ideas! Cheers Sam
            Hide
            samhemelryk Sam Hemelryk added a comment -

            One more possibility:
            Write a doc on overriding the core renderer to produce the menu using another menu system (one we write or any other third party solution).
            This of course would be a solution theme's could implement if they choose and probably wouldn't be more than a days work to prepare the demo and write the doc.

            Show
            samhemelryk Sam Hemelryk added a comment - One more possibility: Write a doc on overriding the core renderer to produce the menu using another menu system (one we write or any other third party solution). This of course would be a solution theme's could implement if they choose and probably wouldn't be more than a days work to prepare the demo and write the doc.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            http://developer.yahoo.com/yui/3/node-menunav/

            Details the YUI3 menunav component

            Show
            samhemelryk Sam Hemelryk added a comment - http://developer.yahoo.com/yui/3/node-menunav/ Details the YUI3 menunav component
            Hide
            samhemelryk Sam Hemelryk added a comment -

            ping

            Show
            samhemelryk Sam Hemelryk added a comment - ping
            Hide
            samhemelryk Sam Hemelryk added a comment - - edited

            Current support:

            anomaly
            arialist
            binarius
            boxxie
            brick
            formal_white
            formfactor
            fusion
            leatherbound
            magazine
            nonzero
            overlay
            serenity
            sky_high
            splash

            Show
            samhemelryk Sam Hemelryk added a comment - - edited Current support: anomaly arialist binarius boxxie brick formal_white formfactor fusion leatherbound magazine nonzero overlay serenity sky_high splash
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Attached screenshots of Arialist, binarius, and boxxie themes

            Show
            samhemelryk Sam Hemelryk added a comment - Attached screenshots of Arialist, binarius, and boxxie themes
            samhemelryk Sam Hemelryk made changes -
            Attachment theme.23188.arialist.jpg [ 21255 ]
            Attachment theme.23188.binarius.jpg [ 21256 ]
            Attachment theme.23188.boxxie.jpg [ 21257 ]
            Hide
            ptrkmkl Patrick Malley added a comment -

            Sam - I think your implementation of custom menus in these three themes works well. Thank you for moving on that and I'm sorry we didn't get to it before you did. We're trying to get to our twenty themes before addressing smaller issues. I'm sure you have a lot of other things on your plate. I'll take care of adding custom menus to leatherbound, nonzero, serenity, and anomaly.

            Show
            ptrkmkl Patrick Malley added a comment - Sam - I think your implementation of custom menus in these three themes works well. Thank you for moving on that and I'm sorry we didn't get to it before you did. We're trying to get to our twenty themes before addressing smaller issues. I'm sure you have a lot of other things on your plate. I'll take care of adding custom menus to leatherbound, nonzero, serenity, and anomaly.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Ok, cool thanks Patrick.

            I'll assign this issue to you then so you are able to track those last four themes.
            Let me know if there is anything I can help with there.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Ok, cool thanks Patrick. I'll assign this issue to you then so you are able to track those last four themes. Let me know if there is anything I can help with there. Cheers Sam
            samhemelryk Sam Hemelryk made changes -
            Assignee Sam Hemelryk [ samhemelryk ] Patrick Malley [ ptrkmkl ]
            dougiamas Martin Dougiamas made changes -
            Fix Version/s 2.0.1 [ 10420 ]
            Fix Version/s 2.0 [ 10122 ]
            dougiamas Martin Dougiamas made changes -
            Workflow jira [ 37419 ] MDL Workflow [ 46448 ]
            dougiamas Martin Dougiamas made changes -
            Fix Version/s 2.0.2 [ 10421 ]
            Fix Version/s 2.0.1 [ 10420 ]
            dougiamas Martin Dougiamas made changes -
            Fix Version/s 2.0.3 [ 10537 ]
            Fix Version/s 2.0.2 [ 10421 ]
            Hide
            ptrkmkl Patrick Malley added a comment -

            Mary - could you please test the core themes for custom menu support and report back what themes still need custom menus added? We should take care of this as soon as possible.

            Show
            ptrkmkl Patrick Malley added a comment - Mary - could you please test the core themes for custom menu support and report back what themes still need custom menus added? We should take care of this as soon as possible.
            ptrkmkl Patrick Malley made changes -
            Assignee Patrick Malley [ ptrkmkl ] Mary Evans [ lazydaisy ]
            Hide
            lazydaisy Mary Evans added a comment -

            Hi Patrick,
            All Moodle core themes support the custom menu, apart from Anomaly and Serenity.

            Cheers
            Mary

            Show
            lazydaisy Mary Evans added a comment - Hi Patrick, All Moodle core themes support the custom menu, apart from Anomaly and Serenity. Cheers Mary
            ptrkmkl Patrick Malley made changes -
            Assignee Mary Evans [ lazydaisy ] John Stabinger [ epsd ]
            epsd John Stabinger made changes -
            Link This issue will be resolved by PULL-696 [ PULL-696 ]
            epsd John Stabinger made changes -
            Link This issue will be resolved by PULL-697 [ PULL-697 ]
            epsd John Stabinger made changes -
            Status Open [ 1 ] Ready for review [ 10010 ]
            Resolution Fixed [ 1 ]
            dougiamas Martin Dougiamas made changes -
            Workflow MDL Workflow [ 46448 ] MDL Full Workflow [ 76453 ]
            Hide
            dougiamas Martin Dougiamas added a comment -

            If only Serenity did, then Anomaly could truly live up to its name.

            I'm reopening this because it seems it did not get integrated this week.

            Show
            dougiamas Martin Dougiamas added a comment - If only Serenity did, then Anomaly could truly live up to its name. I'm reopening this because it seems it did not get integrated this week.
            dougiamas Martin Dougiamas made changes -
            Status Waiting for review [ 10010 ] Closed [ 6 ]
            dougiamas Martin Dougiamas made changes -
            Resolution Fixed [ 1 ]
            Status Closed [ 6 ] Reopened [ 4 ]
            Hide
            dougiamas Martin Dougiamas added a comment -

            Please note that as of this week we have a new (simpler) pull process. All the workflow is in this bug now - no need to create PULL issues anymore.

            Show
            dougiamas Martin Dougiamas added a comment - Please note that as of this week we have a new (simpler) pull process. All the workflow is in this bug now - no need to create PULL issues anymore.
            Hide
            epsd John Stabinger added a comment -

            First time doing the new pull process, hope this is filled out correctly... Thanks!

            Show
            epsd John Stabinger added a comment - First time doing the new pull process, hope this is filled out correctly... Thanks!
            epsd John Stabinger made changes -
            Status Reopened [ 4 ] Waiting for review [ 10010 ]
            Pull Master Diff URL https://github.com/epsd/moodle/compare/master...MDL-23188-3
            Pull Master Branch MDL-23188-3
            Pull 2.0 Diff URL https://github.com/epsd/moodle/compare/master...MDL-23188-3-MOODLE_20_STABLE
            Pull 2.0 Branch MDL-23188-3-MOODLE_20_STABLE
            Pull from Repository git://github.com/epsd/moodle.git
            Fix Version/s 2.1 [ 10370 ]
            Fix Version/s 2.0.2 [ 10421 ]
            Fix Version/s 2.0.3 [ 10537 ]
            Testing Instructions This adds custom menu support to themes Anomaly and Serenity. To test, choose Anomaly and Serenity as themes and confirm that the custom menu appears and works.
            stronk7 Eloy Lafuente (stronk7) made changes -
            Labels ci pullweek-2011-18,
            stronk7 Eloy Lafuente (stronk7) made changes -
            Labels ci pullweek-2011-18, ci pullweek-2011-18
            samhemelryk Sam Hemelryk made changes -
            Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
            Integrator samhemelryk
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks John, this has been integrated now.

            A couple of things to remember:

            1. You need to configure your editor to insert tabs instead of spaces (part of our coding guidelines is that we don't use tabs, we use 4 spaces).
            2. Your 20 stable diff URL is comparing MDL-23188-3-MOODLE_20_STABLE to master instead of MOODLE_20_STABLE.

            I tidied up the whitespace as part of the merge, otherwise spot on.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks John, this has been integrated now. A couple of things to remember: You need to configure your editor to insert tabs instead of spaces (part of our coding guidelines is that we don't use tabs, we use 4 spaces). Your 20 stable diff URL is comparing MDL-23188 -3-MOODLE_20_STABLE to master instead of MOODLE_20_STABLE. I tidied up the whitespace as part of the merge, otherwise spot on. Cheers Sam
            samhemelryk Sam Hemelryk made changes -
            Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
            nebgor Aparup Banerjee made changes -
            Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
            Tester nebgor
            Hide
            nebgor Aparup Banerjee added a comment -

            Thanks! tested on master and 2.x the menus look and work great!

            Show
            nebgor Aparup Banerjee added a comment - Thanks! tested on master and 2.x the menus look and work great!
            nebgor Aparup Banerjee made changes -
            Status Testing in progress [ 10011 ] Tested [ 10006 ]
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Closing as fixed. Many thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Closing as fixed. Many thanks!
            stronk7 Eloy Lafuente (stronk7) made changes -
            Status Tested [ 10006 ] Closed [ 6 ]
            Fix Version/s 2.0.3 [ 10537 ]
            Fix Version/s 2.1 [ 10370 ]
            Fix Version/s 2.0.2 [ 10421 ]
            Resolution Fixed [ 1 ]
            stronk7 Eloy Lafuente (stronk7) made changes -
            Labels ci pullweek-2011-18 pullweek-2011-18
            stronk7 Eloy Lafuente (stronk7) made changes -
            Integration date 4/May/11
            lazydaisy Mary Evans made changes -
            Link This issue will help resolve MDL-22873 [ MDL-22873 ]
            rlnick Nicholas McJetters made changes -
            Link This issue has been marked as being related by MDL-40014 [ MDL-40014 ]

              People

              • Votes:
                1 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  5/May/11