DmitryEfimenko / TwitterBootstrapMvc

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

Default CSS class for validation messages is incorrect in Bootstrap 3 #69

Open JustinPierce opened 10 years ago

JustinPierce commented 10 years ago

When rendering a validation message, BMVC adds the class help-inline to the <span> tag wrapped around the message. This class no longer appears to be relevant in Bootstrap 3. The only help-related class in the Bootstrap 3 source is help-block.

I can work around the issue by explicitly requesting a block-level validation message, but it really should be the default for Bootstrap 3.

DmitryEfimenko commented 10 years ago

Made quite a significant change: removed the whole wrapper div from the validation messages. Reason: div with class help-block adds unwanted in this case margin top and margin bottom.

JustinPierce commented 10 years ago

Actually, that help-block class is supposed to be there. Without it, the has-error class on the form-group won't properly apply the error styling to the help text (the help text will be black instead of red). Additionally, I actually wanted the margins it adds. :wink:

See the BS3 documentation here: http://getbootstrap.com/css/#forms-help-text

DmitryEfimenko commented 10 years ago

It is true that has-error class has effect on help-block. However, I didn't even notice it because the default project creates a css file with default validation related css rules:

/* styles for validation helpers */
.field-validation-error {
    color: #e80c4d;
    font-weight: bold;
}

.field-validation-valid {
    display: none;
}

input.input-validation-error {
    border: 1px solid #e80c4d;
}

.validation-summary-errors {
    color: #e80c4d;
    font-weight: bold;
    font-size: 1.1em;
}

.validation-summary-valid {
    display: none;
}

Just tweak these to match error colors to Bootstrap colors and you are good to go. I think this is a better solution than messing with margins.

JustinPierce commented 10 years ago

So now we can specify .ShowValidationMessage(true, HelpTextStyle.Block), but it doesn't do anything?

DmitryEfimenko commented 10 years ago

The helper that takes two parameters (second parameter HelpTextStyle.Block) should actually be removed from TB3 version since Bootstrap 3 does not have any other help text styles anymore.

Here is the thing about validation error span: Case 1 By default this span will render on all FormGroup elements. If you inspect element you will see:

<span class="field-validation-valid" data-valmsg-for="Password" data-valmsg-replace="true"></span>

So using .ShowValidationMessage(true) in this case indeed won't change anything. On the other hand .ShowValidationMessage(false) will get rid of this error span and therefore even if there is an error for the Password field, the error won't be visible next to the input.

Case 2 using simple input helper (not form group) : @Html.Bootstrap().TextBox() In this case by default that validation span will not be rendered. You can use .ShowValidationMessage(true) and then it will render the span next to the input.

Does this make sense?

JustinPierce commented 10 years ago

I understand the usage. I was just making sure that the HelpTextStyle wasn't supposed to have any effect anymore.

DmitryEfimenko commented 10 years ago

Good. I'll remove that helper overload from the TB3 version

DmitryEfimenko commented 10 years ago

This overload is is removed from TB3

Jak3b0 commented 10 years ago

The code I was using before BMVC was setting the "help-block" class to the span object and not on a div that was wrapping the text.

In my case, I just removed the default CSS since I want to use TwitterBootstrap instead. We might have to test this more but I don't think that it was adding margins when using it like I was...

JustinPierce commented 10 years ago

The help-block class adds the following styles:

.help-block {
  display: block; // account for any element using help-block
  margin-top: 5px;
  margin-bottom: 10px;
  color: lighten(@text-color, 25%); // lighten the text some for contrast
}

I still believe that this class should be applied to the inline help, as it seems clear both from the documentation and behavior of BS3 (help-block reflects the error state of the form group) that this is the intended usage. However, as I have a working alternative, I'm not going to get upset about it, either.

Jak3b0 commented 10 years ago

I agree with Justin, the help-block should be there. If people doesn't like the margins, I think they should override the .help-block style and set what they want.

Because that way, I can easily get a Twitter theme and simply load it and the error color will reflect the one that was set in the theme.

DmitryEfimenko commented 10 years ago

The default BS3 behavior assumes that help-block element is never empty. The initial purpose of this element, as I understand it, is to add some description of what the input is all about. It was just my decision to use this as a container for a validation message. I'd even say that it's a small oversight that a validation error specific class does not exist in BS. Perhaps, I could open an Issue on BS about that.

The problem then with overriding margins for a help-block is that if you decide to use it as originally intended with say an extension method .HelpText(), you will get that description with wrong margins.

Basically, no matter how you look at it, the best solution would be introduction of a new errpr-specific css class by BS.

Jak3b0 commented 10 years ago

You have a point here. I too assumed that the help-block was to show a validation message. Maybe it was because the color was changing when the group was in error.

Nonetheless, I think that BMVC should follow the bootstrap behavior and have the help-block set. That way, we will have the same output as if we were using the bootstrap alone.

What do you guys think?

JustinPierce commented 10 years ago

Now that I understand Dmitry's reasoning, I actually think the validation message should stay the way that it is, with .HelpText() keeping the semantic association with help-block. Even though help-block does change color to match the form group's error state, I'm no longer convinced that that was intended to convey the error state and isn't simply for consistency with the label.

I wouldn't be opposed to a way to tack additional class names onto the validation message, however.

Jak3b0 commented 10 years ago

So, why not revert back to the way Dmitry was doing it before then? Meaning that there is a HelpTextStyle with the option to set the help-block or not as a parameter for the .ShowValidationMessage(). That way, we can still have the behavior as the Boostrap and the choice to opt-out of it and have the validation message be shown as normal text.

I think a lot of people will be expecting the .ShowValidationMessage() to show the message in red as this is the kind of example I've seen a lot on the Internet. I know we can acheive with .HelpText() but having this option with the shortcut of extracting the message automatically is a win-win situation.

DmitryEfimenko commented 10 years ago

HelpTextStyle enum is not relevant in BS3 because help-inline class was removed from it and that was the only other option in it besides help-block.

Jak3b0 commented 10 years ago

Oh, sorry... It's only been 24 hours since I started looking into your library. :) I will suggest a boolean parameter then.

JustinPierce commented 10 years ago

A boolean parameter doesn't really make sense, either, though. The library already has a .HelpText() method that will render a <span class="help-block"> (and if it doesn't, it should; I haven't checked). I think a having a way to attach a custom class name to the validation message would be a happy medium. That way, you could manually set it to help-block, or some custom class of your choosing.

Jak3b0 commented 10 years ago

Sorry, I misread your last post...

Having a way to attach a custom class name would be ok. But it there really time that you will not want the validation message to follow the error status of the form-group? For me, using ShowValidationMessage() is like using HelpText() but with the shortcut of having the validaton message extracted automatically for me.

If the option to specify a custom class for the ShowValidationMessage() is chosen, I think that the HelpText() should follow the same rule.

What do you guys think?