Not having read your code, is there any particular reason why you didn't use the navigation sample from the umbraco template editor?

@inherits umbraco.MacroEngines.DynamicNodeContext
@{ 
  var level = String.IsNullOrEmpty(Parameter.Level) ? 1 : int.Parse(Parameter.Level); 
  var ulClass = String.IsNullOrEmpty(Parameter.UlClass) ? "" : String.Format(" class=\"{0}\"", Parameter.UlClass); 
  var parent = @Model.AncestorOrSelf(level);
  if (parent != null) {
    <[email protected](ulClass)>
    @foreach (var item in parent.Children.Where("Visible")) {
      var selected = Array.IndexOf(Model.Path.Split(','), item.Id.ToString()) >= 0 ? " class=\"selected\"" : "";
      <[email protected](selected)>
        <a href="@item.Url">@item.Name</a>
      </li>
      }
    </ul>
  }
}

Ps. In Umbraco 4.7.1 you will be able to do a foreach instead of a for-loop and check c.IsFirst() and c.IsLast().

Although I really would do that in Javascript most of the time. :)

i'm curious what are your reasons not using umbNaviHide and then use ("Visible") instead of hideInMenu?

@Sebastiaan I didn't know the razor menu sample in the template editor was this extended. I had a look at this and thought that was the only sample available. I'll see if I can use parts of that sample. Thanks. The IsFirst and IsLast extensions seem really usefull in 4.7.1!

@Eran We've got some nodes we want to hide in the menu, but display in the sitemap. So I have a umbracoNaviHide property for hiding in sitemaps and an hideInMenu property for hiding in the menu.

Jeroen

After looking at the navigation sample from the umbraco template editor I made a few changes. Here is my improved code:

@inherits umbraco.MacroEngines.DynamicNodeContext
@using umbraco.MacroEngines;
@using System.Linq;

@{
var top = Model.AncestorOrSelf(1);
var classextender = "";
var count = 0;
var childrenCount = top.Children.Where("hideInMenu != true").Count();

    

    @foreach (var c in top.Children.Where("hideInMenu != true"))
    {
        //Check if it's the first or last item in the loop and add a class.
        if(count == 0)
        {
            classextender = " class=\"start{0}\"";
        }
        else if(count == childrenCount-1)
        {
            classextender = " class=\"end{0}\"";
        }

        //Check if the id  we are looping through is part of the path.
        if (Array.IndexOf(Model.Path.Split(','), c.Id.ToString()) >= 0)
        {
            //Add the active class.
            classextender = string.IsNullOrWhiteSpace(classextender) ? " class=\"active\"" : string.Format(classextender, " active");
        }
        else if (!string.IsNullOrWhiteSpace(classextender))
        {
            //Remove the {0} from the classextender if the page isn't active.
            classextender = string.Format(classextender, string.Empty);
        }

        @c.Name
        classextender = "";
        count++;
    }
    
}

Instead of doing:

Model.AncestorOrSelf(2).Id == c.Id 

I now use:

Array.IndexOf(Model.Path.Split(','), c.Id.ToString()) >= 0

And I also added some lines together.

Jeroen

Just happened on this thread. According to my tests (on a smallish site) then:

string selected = page.IsAncestorOrSelf(Model, " class=\"selected\"", null);

is not only far more readable but is also significantly faster than:

string selected = Array.IndexOf(Model.Path.Split(','), page.Id.ToString()) >= 0 ? " class=\"selected\"" : "";

You may get different results with a large site, but this was my experience (using StopWatch to time rendering).

I don't even see how checking the index of curent page, converting it to a string and comparing it to "0" tests that your page is "current page" (Model).

If someone could explain this statement it would really help me out:

if(Array.IndexOf(Model.Path.Split(','), c.Id.ToString())>=0) ? " class=\"selected\"" : "";

Umbraco stores a "path" parameter for every page that contains the IDs of the page you are on and all it's ancestors as a comma-delimited list eg "-1, 10, 18, 1024". If you split that list on a comma you get a string array of all those IDs. The Array.IndexOf method then returns the index of the first occurence of the value you are checking (ie. the ID of the current page). So if it returns > 0 then you know that your ID was in the array and therefore in the path list, meaning the ID being checked must be the current page or an ancestor of the current page.

I still think IsAncestorOrSelf is much neater, though :)

Thanks Dan!

That helps alot. I've been using IsAncestorOrSelf, but most of the time I need to check for if the node being rendered is "current page" and the only way for that seems to be "IsEqual(Model)".

is working on a reply...