erichexter / twitter.bootstrap.mvc

nuget package to make bootstrap easy with mvc4
Apache License 2.0
248 stars 134 forks source link

Added a toggle button. #81

Closed Wakusei closed 9 years ago

Wakusei commented 11 years ago

Hi, I added a toggle button with the bootstrap style. If you think it's a good idea to include, pull the commit.

erichexter commented 11 years ago

Please fill out the contributor agreement. http://sdrv.ms/13eMRXm so that I can land this pull request.

serra commented 11 years ago

@Wakusei Thanks for this. I like your idea of supporting bootstrap-styled toggle buttons in the html helpers package. I also like that your implementation is fairly complete; you provide a bundled script and helpers for getting an MvcHtmlString and a complete editor using ToggleButtonFor helper method.

There are some aspects about this pull request that need improving though, please review the following points which IMO should be addressed before considering merging this pull, please review them and update your pull request; I'll be happy to review it again.

1) use twitter.bootstrap toggle styling

Twitter.Bootstrap supports a button toggle style out-of-the box and it is this style that we should leverage; check out the TB docs/javacscript. So a toggle button should be rendered using the data-toggle like:

<!-- not checked: -->
<button type="button" class="btn" data-toggle="button">Single Toggle</button>
<!-- checked: -->
<button type="button" class="btn active" data-toggle="button">Single Toggle</button>

By adding or removing the active class to this button, the rendering is done. Check out mentioned the docs and be sure to use your inspector to check out the dom of the toggle buttons there.

2) the javascript you provided breaks when adding additional elements to the editor-field

For instance this razor view breaks you javascript:

<div class="editor-label">
    @Html.LabelFor(model => model.IsOn)
</div>
<div class="editor-field">
    <a href="www.github.com">github</a>
    @Html.ToggleButtonFor(model => model.IsOn)
    @Html.ValidationMessageFor(model => model.IsOn)
</div>
3) generate valid html only

right now, the ToggleButtonFor generates html similar to

<div class="btn" bshelper_act="btn-info" bshelper="toggle">
  IsOn
</div>
<input id="IsOn" type="checkbox" value="true" style="visibility:hidden;" name="IsOn"></input>

afaik the bshelper_act and bsheler attributes are not valid html; if you want to use custom attributes, use the data- attributes, which are intended for this use; e.g. data-bshelper-act=...

erichexter commented 9 years ago

closing since the concerns in the review were not addressed.