Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Navigation Overhaul #68

Merged
merged 1 commit into from
Feb 20, 2013
Merged

Navigation Overhaul #68

merged 1 commit into from
Feb 20, 2013

Conversation

MisterJames
Copy link
Collaborator

Built in support for the concept of "NavigationRouteOptions" to clean up
the code smell of "ShouldBreakAfter". Refactored the navigation list
builder to support groups. Added support for "pull-right" on menu group
ULs. Moved area information into the NavigationRouteOptions class.
Updated and added tests as necessary. Updated samples, added comments
and example route filter to sample project.

Built in support for the concept of "NavigationRouteOptions" to clean up
the code smell of "ShouldBreakAfter". Refactored the navigation list
builder to support groups. Added support for "pull-right" on menu group
ULs. Moved area information into the NavigationRouteOptions class.
Updated and added tests as necessary. Updated samples, added comments
and example route filter to sample project.
@MisterJames
Copy link
Collaborator Author

This is a bigger one, likely further evidence that NavigationRoutes should be broken out to their own home.

Biggest change is in cleaning up how we were passing in params to the route overloads...any additional options would have to - again and again - be refactored into the overloads. I created a "NavigationRouteOptions" class for this to compensate, example use in the sample project.

The pull-right support...I added this so that a user would be able to, say, put links relevant to the logged in user into a menu group and pull them over to the right. I'll add a quick sample to this sometime soon.

@erichexter
Copy link
Owner

Totally agree that nav routes needs to be broken out and included as an assembly rather then code. The biggest feature request thats been asked for at this point is an example and an api for applying filters. There is a unit test demonstrating the concept but no api for adding filters

@MisterJames
Copy link
Collaborator Author

Yeah...API would be cool. It's not really an API at this point...I'd like to work a bit further and expose a FilterCollection or something with some smarts.

As an example of where I can see this being used - and how I'm using it - is in having a plugin-type approach where the plugins can register routes. Of course, that means there would be some "admin" type functions that should be grouped under existing menus, as in, "I want to add my 'Configure My Plugin' link under the 'Administration' menu item."

Also, not sure how you feel about the FilterToken, but I needed a way to get more information about the state of a menu item when the filters were being execute, but I wanted to keep it simple as well. Open to feedback there.

@serra serra mentioned this pull request Feb 8, 2013
3 tasks
erichexter added a commit that referenced this pull request Feb 20, 2013
Navigation Overhaul - lets see how this goes.
@erichexter erichexter merged commit e821a0b into erichexter:master Feb 20, 2013
@@ -13,11 +14,21 @@ public class ExampleLayoutsRouteConfig
{
public static void RegisterRoutes(RouteCollection routes)
{
routes.MapNavigationRoute<HomeController>("Automatic Scaffolding", c => c.Index(), "", true);
// this enables menu suppression for routes with a FilterToken of "admin" set
NavigationRouteFilters.Filters.Add(new AdministrationRouteFilter());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those who want to get the role based navigation menu working properly should pay special attention to this code line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants