DmitryEfimenko / TwitterBootstrapMvc

Fluent implementation of ASP.NET-MVC HTML helpers for Twitter Bootstrap.
Apache License 2.0
224 stars 79 forks source link

Setting Active on BootstrapLink inside a Nav #416

Open ghost opened 8 years ago

ghost commented 8 years ago

First of all thanks for the wonderful work you have put into this!

I might be missing something, but I think it's not possible to set the class Active on a link inside a nav? I was hoping that on a BootstrapLink a method Active(bool active = true) existed, just like the method Disabled(bool disabled = true) exists. I understand the methods SetLinksActiveByController() and SetLinksActiveByControllerAndAction() exist on a Nav, but they can only be used if the navs are used within the context of controllers and actions.

In my case, a nav is used for pagination, and it would be great if one could set Active on a BootstrapLink, just like Disabled is used.

Indended markup:

<ul class="nav pagination">
        <li><a href="#" >previous</a></li>
        <li><a href="#">1</a></li>
        <li class="active"><a href="#">2</a></li>
        <li><a href="#">3</a></li>
        <li class="disabled"><a href="#" class="disabled ">...</a></li>
        <li><a href="#">25</a></li>
        <li><a href="#">next</a></li>
</ul>
DmitryEfimenko commented 8 years ago

could you please show me desired BMVC code that you'd have?

ghost commented 8 years ago

Hey, sure! Here it goes:

using (var nav = Html.Bootstrap().Begin(new Nav().Class("pagination")))
{
    @nav.Link("previous", "#")
    @nav.Link("1", "#")
    @nav.Link("2", "#").Active()
    @nav.Link("3", "#")
    @nav.Link("...", "#").Disabled()
    @nav.Link("25", "#")
    @nav.Link("next", "#")
}

By the way, the Disabled() renders class="disabled" on both the li and the a. Setting it on the li only is fine: http://getbootstrap.com/components/#nav-disabled-links

DmitryEfimenko commented 8 years ago

you could set class via .Class() method:

@nav.Link("2", "#").Class("active")

Would that solve this issue?

ghost commented 8 years ago

No, that sets the active class on the a. Bootstraps Navs need to have the class active set on the li.

DmitryEfimenko commented 8 years ago

Right. Now, before I proceed adding this feature I just want to make sure it'll solve your problem. What is the logic that you are using to determine what link should be active? Is it a value your ViewModel? Something like curentPage? If so, you'd have to put .Active() method on all links:

@nav.Link("1", "#").Active(Model.CurentPage == 1)
@nav.Link("2", "#").Active(Model.CurentPage == 2)
@nav.Link("3", "#").Active(Model.CurentPage == 3)

... which is a bit ugly. Unless you have a different use case.

ghost commented 8 years ago

No, the links are rendered in a for loop. But I will spare you the details of the complex pagination/UI requirements ;-)

But I also ran into this when rendering a set of navigation pills to navigate to a subsection of a website. This site is powered by Umbraco, hence not within MVC controller/action context. A foreach loop that goes over the available pages in the subsection renders the pills.

And offcourse, setting the class=active on the a is an option. But then one would still need to tweak the Bootstrap CSS, to adjust for BMVC's rendering.

I think it's comparable with how you implemented the Disabled() method.

DmitryEfimenko commented 8 years ago

This is done. Please get latest. Although there is no new .Active() method. Go ahead and apply class "active" to the desired link. It'll know to also apply it to the wrapping <li> as well.

@nav.Link("2", "#").Class("active")

I didn't go with the .Active() method to avoid polluting extension methods. This method would not quite make sense in the context of a regular Link or ActionLink

Let me know if it worked for you.

ghost commented 8 years ago

I understand, and I guess the BootstrapLink is also the baseclass for Link or ActionLink? Thanks though! Works fine, just grabbed the latest from NuGet. Thanks for your swift response!

DmitryEfimenko commented 8 years ago

Not quite even though it would be logical. I initially was thinking about doing that, but some complications were introduced by the fact that these helpers also needed to work under .FormGroup() context. Anyway, I will spare you the details of the complexity of the implementation that I have in place ;)

ghost commented 8 years ago

Haha, I had a brief look under the hood and can imagine so. Would have been nice to have a method that follows the same pattern as Disabled, but understand your point. Created my own extension method for it now, works fine for this case.