form validation css support

manuel_84 avatarmanuel_84 created an issue

I've just noticed that only the Horizontal form is supporting style for inline/ajax validation We should add it to vertical and the others

The problem is that this code is missing in BootInput Vertical/Inline, why?

/**
	 * Runs the widget.
	 */
	public function run()
	{
		echo CHtml::openTag('div', array('class'=>'control-group '.$this->getContainerCssClass()));
		parent::run();
		echo '</div>';
	}

and this div wrapping is missing too:

  echo '<div class="controls">';
...
  echo '</div>';

should I make a PR?

Comments (16)

  1. Christoffer Niska

    Well Bootstrap vertical forms doesn't have control-groups or controls since that would break, e.g. inline forms. I'd suggest using errorSummary instead of displaying errors for each field.

  2. Christophe BOULAIN

    Bootstrap doc says that Vertical form should be "lighter and smarter", with "no extra markup"... I find that a bit restrictive. I totally agree for inline forms, which are designed to be compact, and therefore, an errorSummary displayed on submit is sufficient.

    But for Vertical Forms, my opinion is that we should have a surrounding tag (control-group ?) at least to allow to style errors like in Horizontal forms. The drawback is that .control-group class has a default margin-bottom set to 9px (@baseLineHeight/2), but, if needed, can be easily fixed in own style sheet.

    To sum up, I would vote to add this piece of code in BootInputVertical :

    public function run()
    {
    	echo CHtml::openTag('div', array('class'=>'control-group '.$this->getContainerCssClass()));
    	parent::run();
    	echo '</div>';
    }
    

    And leave BootInputInline as is (i.e. no ajax validation, only on submit with errorSummary)

  3. Christophe BOULAIN

    I don't think wrapping controls is useful for Vertical forms (except maybe for radio/checkbox). If you look at bootstrap css (or at forms.less), you'll see that .controls class is only used in .form-horizontal.

  4. manuel_84

    I was thinking that too before seeing this http://www.markdotto.com/2011/10/10/help-us-refactor-forms-for-bootstrap-2-0/ and from the source-code of twitter.github.com/bootstrap/base-css.html#forms there's this vertical-form:

          <form>
            <div class="control-group">
              <label class="control-label" for="inputIcon">Email address</label>
              <div class="controls">
                <div class="input-prepend">
                  <span class="add-on"><i class="icon-envelope"></i></span>
                  <input class="span2" id="inputIcon" type="text">
                </div>
              </div>
            </div>
    

    The result with or without the .controls div look the same, so we can leave it.

    Consider that BootInputInline extend from BootInputVertical so we need a check before adding .control-group

    So I would add this code to BootInputVertical

    	public function run($inline=false)
    	{
    		if (!$inline) echo CHtml::openTag('div', array('class'=>'control-group '.$this->getContainerCssClass()));
    		parent::run();
    		if (!$inline) echo '</div>';
    	}
    

    And this to BootInputInline

    	public function run($inline=true)
    	{
    		parent::run($inline);
    	}
    
  5. Christophe BOULAIN

    You're right for BootInputInline, but I wouldn't change CWidget::run() signature in derived classes :)

    Just put this in BootInputInline :

    public function run ()
    {
        BootInput::run();
    }
    

    But i'm thinking of another approach, more generic :

    In BootInput base class:

    public function run ()
    {
      $this->beginControlGroup()
      switch ($this->type)
      {
        // current code
      }
      $this->endControlGroup()
    }
    
    protected function beginControlGroup ()
    {
      echo CHtml::openTag ('div', array('class'=>'control-group '.$this->getContainerCssClass()));
    }
    
    protected function endControlGroup ()
    {
      echo "</div>";
    }
    

    and override the begin/endControlGroup method in BootInputInline.

    but let's wait Chris opinion on this. It's his extension ! :)

  6. Christoffer Niska

    Lets say that we'd add control-groups for vertical forms, in that case I would also wrap the controls in an own div to stay consistent. I've been thinking about always making the success/warning/error inline in horizontal forms (because that's how it is in their docs), this would make everything more simple. Then on the other hand we could use success/warning/error blocks in vertical forms. That would add a bit more flexibility to the forms imho.

    However, I wouldn't change the run signature. Instead I wouldn't extend the inline form from the vertical one and change the code in the run methods to generate the correct code.

    BTW, what do you two think about the current form implementation, is it good enough? I think there is still room for improvement but overall it works quite nicely and it's quite easy to change the code without breaking BC.

    I'm also planning to write a CSS file that would sort of bridge Bootstrap and Yii by adding the missing CSS such as .required, grid styles, etc. What do you think about that?

    Maybe we should start a google group instead of discussing the development here in the issues? :)

  7. Anonymous

    I understand this is a problem of original bootstrap's intention and bootstrap css and yii-bootstrap is only facilitating this. However, ideally the entire decision of displaying errors in horizontal/inline/vertical forms is left to the developer. Lets say there's error styles in place for all 3 form types - control-group for horizontal and vertical; and something else that only does the focus of the form element with no additional help-inline for inline forms. Then we could add another parameter to BootsFormAction named "displayErrors" where the default value is true for horizontal and false for inline and vertical. Just an idea... cheers, yiidf

  8. Log in to comment
Tip: Filter by directory path e.g. /media app.js to search for public/media/app.js.
Tip: Use camelCasing e.g. ProjME to search for ProjectModifiedEvent.java.
Tip: Filter by extension type e.g. /repo .js to search for all .js files in the /repo directory.
Tip: Separate your search with spaces e.g. /ssh pom.xml to search for src/ssh/pom.xml.
Tip: Use ↑ and ↓ arrow keys to navigate and return to view the file.
Tip: You can also navigate files with Ctrl+j (next) and Ctrl+k (previous) and view the file with Ctrl+o.
Tip: You can also navigate files with Alt+j (next) and Alt+k (previous) and view the file with Alt+o.