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

Added a toggle button. #81

Closed
wants to merge 3 commits into from
Closed

Added a toggle button. #81

wants to merge 3 commits into from

Conversation

Wakusei
Copy link

@Wakusei Wakusei commented Mar 10, 2013

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

@erichexter
Copy link
Owner

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

@serra
Copy link
Contributor

serra commented Mar 12, 2013

@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
Copy link
Owner

closing since the concerns in the review were not addressed.

@erichexter erichexter closed this Nov 13, 2014
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