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

Implementation of breadcrumbs #58

Closed
wants to merge 1 commit into from
Closed

Conversation

jensj
Copy link

@jensj jensj commented Jan 27, 2013

Proposal for implementation of breadcrumbs.

Worth noticing is that in the sample project there is a need to change the install script to add routes in order for the named routes to show up in RouteData.

BootstrapSupport.BootstrapBundleConfig.RegisterBundles(System.Web.Optimization.BundleTable.Bundles);
RouteConfig.RegisterRoutes(RouteTable.Routes);

Nuget package works fine after swapping above mentioned lines...

@serra
Copy link
Contributor

serra commented Jan 28, 2013

Looks interesting, I will check this out later this week. Some points to consider:

  • I think we should move breadcrumbs html helpers extensions to the html helpers package.
  • There should be a example (or test, or both) on how to configure the parents of named routes (and what's the relation to the Children property).

@erichexter
Copy link
Owner

Yes. please move to the helpers package and add a example and tests if
there were code changes to the navigation routes.

Eric Hexter

blog | http://Hex.LosTechies.com
info | http://www.linkedin.com/in/erichexter

On Mon, Jan 28, 2013 at 5:05 AM, Marijn van der Zee <
notifications@github.com> wrote:

Looks interesting, I will check this out later this week. Some points to
consider:

  • I think we should move breadcrumps html helpers extensions to the html
    helpers packagehttps://github.com/erichexter/twitter.bootstrap.mvc/tree/master/src/Bootstrap/HtmlHelpers
    .

  • There should be a example (or test, or both) on how to configure the
    parents of named routes.


    Reply to this email directly or view it on GitHubhttps://github.com/Implementation of breadcrumbs #58#issuecomment-12777143.

@erichexter erichexter closed this Jan 29, 2013
@serra
Copy link
Contributor

serra commented Jan 29, 2013

@erichexter, why close this pull request - looks like a good idea that only needs some improvements?

@erichexter
Copy link
Owner

I asked to move them to a the helpers package and add a samples. So, I
figured that it was best to close this and wait for an updated pull
request. I am trying to land all of the outstanding pull request this
week, so I closed this knowing that I expected a little change to the
implementation. I think its an awesome idea!

Eric Hexter

blog | http://Hex.LosTechies.com
info | http://www.linkedin.com/in/erichexter

On Tue, Jan 29, 2013 at 1:32 AM, Marijn van der Zee <
notifications@github.com> wrote:

@erichexter https://github.com/erichexter, why close this pull request

  • looks like a good idea that only needs some improvements?


Reply to this email directly or view it on GitHubhttps://github.com//pull/58#issuecomment-12823542.

serra added a commit to serra/twitter.bootstrap.mvc that referenced this pull request Jan 29, 2013
serra added a commit to serra/twitter.bootstrap.mvc that referenced this pull request Jan 29, 2013
@serra
Copy link
Contributor

serra commented Jan 29, 2013

@jensj - I have changed the install script in the sample package to insert the bootstrap support code before RouteConfig.RegisterRoutes(RouteTable.Routes); (as per your comment) and moved the breadcrumbs code to the HtmlHelpers package.

Quickest way for you to pick up these changes (from a git shell in your twitter.bootstrap.mvc repo with a clean working directory) is to do:

git remote add serra https://github.com/serra/twitter.bootstrap.mvc.git
git fetch serra
git checkout breadcrumbs
git merge serra/breadcrumbs

I forked from your breadcrumbs branch, so this should merge easily for you. If not, just let me know. I'm looking into this in more details now, so I might add some more commits, but I'll make sure you can easily pull them to add them to your pull request.

serra added a commit to serra/twitter.bootstrap.mvc that referenced this pull request Jan 29, 2013
@jensj
Copy link
Author

jensj commented Jan 30, 2013

@Serrra, very nice of you. I the reason why I put the breadcrubs together with navigation was that I though of it as a part of navigation.

@serra
Copy link
Contributor

serra commented Feb 1, 2013

I see your point, but it appears that Eric likes to have this kind of code in the HtmlHelpers package. I suggest we keep it in HtmlHelpers for now. We can implement additional Bootstrap components there too; I've added paging (#51) - which will be next? 😄

@serra
Copy link
Contributor

serra commented Feb 1, 2013

@jensj - consider reopening this pull request if you like the state of the code.

@jensj
Copy link
Author

jensj commented Feb 1, 2013

I'll have a look during the weekend. Guess there will be a bunch of other ones to implement 😄

@jensj
Copy link
Author

jensj commented Feb 6, 2013

@serra Hmmm, not finding time to work on this project right now. Most probably I'll be busy the upcoming weeks so feel free to move forward in any way you find appropriate.

@serra
Copy link
Contributor

serra commented Feb 7, 2013

OK, thanks for the status update. You can leave this pull request closed; I'll open a new one.

@serra serra mentioned this pull request Feb 8, 2013
3 tasks
@erichexter
Copy link
Owner

I appreciate your pull request, will please submit this contributor agreement. http://sdrv.ms/13eMRXm this will help keep the project compliant with our open source license.

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