Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major 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:
    • Rank:
      494

      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?

        Issue Links

          Activity

          Hide
          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
          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
          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
          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
          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
          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
          Martin Dougiamas added a comment -

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

          Show
          Martin Dougiamas added a comment - When I visit http://www.newschoollearning.com/moodle/?&theme=afterburner with JS off then the menu is dead...
          Hide
          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
          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
          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
          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?
          Hide
          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
          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
          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
          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
          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
          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
          Sam Hemelryk added a comment -

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

          Details the YUI3 menunav component

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

          ping

          Show
          Sam Hemelryk added a comment - ping
          Hide
          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
          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
          Sam Hemelryk added a comment -

          Attached screenshots of Arialist, binarius, and boxxie themes

          Show
          Sam Hemelryk added a comment - Attached screenshots of Arialist, binarius, and boxxie themes
          Hide
          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
          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
          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
          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
          Hide
          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
          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.
          Hide
          Mary Evans added a comment -

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

          Cheers
          Mary

          Show
          Mary Evans added a comment - Hi Patrick, All Moodle core themes support the custom menu, apart from Anomaly and Serenity. Cheers Mary
          Hide
          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
          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.
          Hide
          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
          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
          John Stabinger added a comment -

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

          Show
          John Stabinger added a comment - First time doing the new pull process, hope this is filled out correctly... Thanks!
          Hide
          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
          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
          Hide
          Aparup Banerjee added a comment -

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

          Show
          Aparup Banerjee added a comment - Thanks! tested on master and 2.x the menus look and work great!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed. Many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed. Many thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: