Closed jensj closed 11 years ago
Looks interesting, I will check this out later this week. Some points to consider:
Children
property).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/erichexter/twitter.bootstrap.mvc/pull/58#issuecomment-12777143.
@erichexter, why close this pull request - looks like a good idea that only needs some improvements?
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/erichexter/twitter.bootstrap.mvc/pull/58#issuecomment-12823542.
@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.
@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.
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? :smile:
@jensj - consider reopening this pull request if you like the state of the code.
I'll have a look during the weekend. Guess there will be a bunch of other ones to implement :smile:
@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.
OK, thanks for the status update. You can leave this pull request closed; I'll open a new one.
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.
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.
Nuget package works fine after swapping above mentioned lines...