seyhunak / twitter-bootstrap-rails

Twitter Bootstrap for Rails 6.0, Rails 5 - Rails 4.x Asset Pipeline
https://github.com/seyhunak/twitter-bootstrap-rails
4.5k stars 996 forks source link

Bootstrap:themed removes validation msg functionality #747

Closed Sprachprofi closed 10 years ago

Sprachprofi commented 10 years ago

Applying rails g bootstrap:themed [RESOURCE] after using one of the standard Rails scaffold generators completely removes the validation feedback part from these forms, for example

<% if @user.errors.any? %>
    <div id="error_explanation">
      <h2><%= pluralize(@user.errors.count, "error") %> prohibited this user from being saved:</h2>

      <ul>
      <% @user.errors.full_messages.each do |msg| %>
        <li><%= msg %></li>
      <% end %>
      </ul>
    </div>
  <% end %>

This leads to bad default UI, because in the event of a validation error, the user is left clicking "Save" time and time again without any feedback on what's wrong, or even that something is wrong.

toadkicker commented 10 years ago

I believe most users opt for a different html structure than the default rails example, using p.help-block inside a .form-group. Global alerts are usually done using bootstrap's javascript alert method. I'm not opposed to a default working implementation, but if it is included it should be more like bootstrap's html.

Sprachprofi commented 10 years ago

This refers specifically to the part of Twitter-Bootstrap-Rails that is intended to be used after using a default Rails generator. See the suggested use in Twitter-Bootstrap-Rails documentation:

rails g scaffold Post title:string description:text
rake db:migrate
rails g bootstrap:themed Posts

In my implementation I used the "danger" panel from http://getbootstrap.com/components/#panels . This is a standard part of Bootstrap and the direct equivalent of what Rails uses. I don't think a Javascript alert would work well in this situation because there are often several validation problems and people need to refer back to them as they're fixing their input.

toadkicker commented 10 years ago

Most users were installing their own validation solutions because they are very specific implementations. I'm not sure if you are concerned about the form templates or displaying an alert, or both. In either case the gem supports it and leaves validation implementation up to you. We could include a popular one and generate an installer for it I suppose. Here if you want give this a shot: https://github.com/toadkicker/twitter-bootstrap-rails/blob/error_msgs/lib/generators/bootstrap/themed/templates/_form.html.erb

I haven't finished testing it. I'm welcome to feedback if you get a chance to grab that branch.

Sprachprofi commented 10 years ago

I am concerned about the form templates. Bootstrap is often used for rapid prototyping. Rails scaffolds + Bootstrap is almost certainly going to be used for rapid prototyping, so it's important to have something that "just works" in the default scenario without requiring a web of different plugins.

Rails scaffolds already includes a way to display validation error messages, so whenever Twitter-Bootstrap-Rails modifies these scaffolds (and only then), they should continue to include the validation error messages. This is not introducing anything new.

I have less marked preference for the way that such error messages are displayed. I think javascript alerts are very user-unfriendly for this kind of information and my favourite solution are Bootstrap alert panels, but if you prefer a different kind of styling, that's fine with me.

toadkicker commented 10 years ago

I agree there should be sensible defaults and displaying an error message regardless of javascript and styling. Both approaches are fine to fix the issue, but in order to finish the PR we'd have to update the three template formats available. Doing it the pure bootstrap way we could just write a helper that adds these classes in when errors are present. What do you think?

Sprachprofi commented 10 years ago

Either way is fine. I still don't see the three template formats, those come in a later version of Twitter-Bootstrap-Rails only.

toadkicker commented 10 years ago

in https://github.com/seyhunak/twitter-bootstrap-rails/tree/master/lib/generators/bootstrap/themed we have slim, erb, and haml available.

toadkicker commented 10 years ago

Closing this and merging your PR for 2.0 branch and taking my branch for 3.1.1.