Follow-up from #1840980: [meta] Bootstrap 3.0.

See: http://getbootstrap.com/css/#forms

Change

.input-prepend, .input-append to .input-group
.add-on to .input-group-addon

New

Input groups .input-group, .input-group-addon, .input-group-btn
Form controls .form-control, .form-group

Removed

.form-actions, .form-search

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

valkum’s picture

Status: Active » Needs work
FileSize
12.87 KB

i started work on this issue.
So far
* i removed .form-search from form.inc

* started merging prepend and append to .input-group. It seems that this doesn't work as i wish.

Every form element needs the .form-control class. The only way in my eyes to do this are preprocess functions for every form element type.
I started with textfield and textarea as examples.
Are there better ways to approach the result?

I'm not sure how to migrate the old controls and control-group correctly.
I just named controls to group and control-group to form-group.

Anonymous’s picture

Works fine for me.

heylookalive’s picture

Status: Needs work » Needs review
FileSize
19.2 KB

Following on from @valkums work, have defined preprocessors for all form elements. Including file.

I've also adjusted the line height in override.css as it was throwing off labels for both radios and checkboxes. Additionally have made some coding standards tweaks

markhalliwell’s picture

Status: Needs review » Needs work
  1. diff --git a/css/overrides.css b/css/overrides.css
    index ec33ece..98d6ad3 100644
    --- a/css/overrides.css
    +++ b/css/overrides.css
    \ No newline at end of file
    

    Don't include the overrides, this will be handed in another issue.

  2. +++ b/includes/form.inc
    @@ -101,10 +98,21 @@ function bootstrap_form_element(&$variables) {
    +  ¶
    

    Whitespace at EOL.

  3. +++ b/includes/form.inc
    @@ -212,6 +238,50 @@ function bootstrap_form_element_label(&$variables) {
     /**
    + * Returns HTML for a date element.
    + */
    +function bootstrap_preprocess_date(&$vars) {
    +  $vars['element']['#attributes']['class'][] ='form-inline';
    +}
    +
    +/**
    + * Returns HTML for a managed file element.
    + */
    +function bootstrap_preprocess_file_managed_file(&$vars) {
    +  $vars['element']['#attributes']['class'][] = 'form-inline';
    +  $vars['element']['upload']['#prefix'] = '<div class="form-group">';
    +  $vars['element']['upload']['#suffix'] = '</div>';
    +}
    +
    +/**
    + * Returns HTML for a password element.
    + */
    +function bootstrap_preprocess_password(&$vars) {
    +  $vars['element']['#attributes']['class'][] ='form-control';
    +}
    +
    +/**
    + * Returns HTML for a select element.
    + */
    +function bootstrap_preprocess_select(&$vars) {
    +  $vars['element']['#attributes']['class'][] ='form-control';
    +}
    +
    +/**
    + * Returns HTML for a textfield element.
    + */
    +function bootstrap_preprocess_textfield(&$vars) {
    +  $vars['element']['#attributes']['class'][] ='form-control';
    +}
    +
    +/**
    + * Returns HTML for a textarea element.
    + */
    +function bootstrap_preprocess_textarea(&$vars) {
    +  $vars['element']['#attributes']['class'][] ='form-control';
    +}
    +
    +/**
    

    Aren't these already handled by bootstrap_form_element()?

    Wouldn't it make sense to do these in one function and throw a switch, instead of costing us PHP execution time by calling more functions?

heylookalive’s picture

Hey Mark, yeah was annoyed that there wasn't a general preprocessor that made sense for all of those items though it's possible I've missed something. From what I understand we want the form-control class to be on the form element itself as opposed to a wrapper (which you can effect in theme_form_element).

Can reroll once we've hashed this out, noted on overrides.css.

markhalliwell’s picture

Looking at system_element_info() and element_info(), I think it would be much cleaner if we just implemented a hook_element_info_alter() on all elements that have #input => TRUE and append our own custom #process function to add these classes.

heylookalive’s picture

Ah, that's what I was thinking of. Yep will go for them and rw supply a patch. Will implement that for the more generic elements that just need that common class.

heylookalive’s picture

Status: Needs work » Needs review
FileSize
9.2 KB

When I did the initial patch I was looking for a better way to do this en masse for most of those functions. Unfortunately hook_element_info_alter() only fires for modules so this isn't viable, and neither is hook_preprocess().

Have updated the patch.

Denes.Szabo’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
14.77 KB

Your path seems fine.

As I see the form theming needs little fix for .help-block below the textareas. As I saw, this misalignment exists on the Bootstrap2 too.

heylookalive’s picture

Hi @denes, yep. As @mike said we'll tackle this in #2084331: Update and recompile the overrides.less/css file. Thanks for the patch review!

Denes.Szabo’s picture

Of course. I added a patch that issue too (#2084331).

markhalliwell’s picture

Status: Reviewed & tested by the community » Needs work

@heylookalive, re:

Unfortunately hook_element_info_alter() only fires for modules so this isn't viable, and neither is hook_preprocess()

This should should work just fine. It's an alter hook (which are invoked on the current theme) did you clear caches when you implemented them? Also, if you are saying that "hook_preprocess() also doesn't work", then why are they in the patch?

heylookalive’s picture

Morning, will double check on hook_element_info_alter but stepped through it and it wasn't happening. And "hook_preprocess" isn't viable whereas we're implementing "hook_preprocess_hook".

heylookalive’s picture

Morning, will double check on hook_element_info_alter but stepped through it and it wasn't happening. And "hook_preprocess" isn't viable whereas we're implementing "hook_preprocess_hook".

heylookalive’s picture

Double post, weird. Just tried that hook again and it's working, my test site must've been screwy in some way. Will work on this patch.

markhalliwell’s picture

@heylookalive, yes drupal_alter() allows themes to alter data.

valkum’s picture

I hope your haven't startet already @heylookalive.
I reworked my patch and implemented the things discussed in the last comments.
Works well in my test setup, less code then all input type preprocessors.
There are maybe one ore two more types where the form-control class shouldn't be added.

valkum’s picture

I'm very sorry for this double post but there is submit and file missing in the list of types which shouldn't have class form-control.

markhalliwell’s picture

Over all very good! A few tiny nitpicks though:

  1. +++ b/includes/form.inc
    @@ -212,6 +212,38 @@ function bootstrap_form_element_label(&$variables) {
    +    if (isset($element['#input']) && $element['#input'] === TRUE) {
    

    We could probably just do: !empty($element['#input'] here.

  2. +++ b/includes/form.inc
    @@ -212,6 +212,38 @@ function bootstrap_form_element_label(&$variables) {
    +  // Dont't put form-control on this input types.
    

    Typos. Replace with:
    Don't inject the "form-control" class on these input types.

  3. +++ b/includes/form.inc
    @@ -212,6 +212,38 @@ function bootstrap_form_element_label(&$variables) {
    +  $types = array(
    ...
    +  if(!in_array($element['#type'], $types)) {
    

    Rename the $types variable to $ignore_types to have more semantic meaning.

valkum’s picture

Status: Needs work » Needs review
FileSize
1.02 KB

Ok here with changes from #19

markhalliwell’s picture

Status: Needs review » Fixed

Thanks @valkum!

Committed 9f38f98 to 7.x-3.x.

valkum’s picture

Status: Fixed » Needs review
FileSize
6.51 KB

I've also done some work on the hook_form_element and hook_append_element function.
Based on the patches from @heylookalive

The rendered HTML is like the one mentioned on http://getbootstrap.com/css/#forms-controls

the $exclude_control isn't need anymore i think.

markhalliwell’s picture

Status: Needs review » Needs work

Hmm, interesting. I'll need to manually review this. Quick review (using Dreditor btw) detected whitespace issues:

+++ b/includes/form.inc
@@ -106,6 +96,18 @@ function bootstrap_form_element(&$variables) {
+  ¶
...
+  } ¶

@@ -339,16 +344,19 @@ function bootstrap_bootstrap_append_element(&$variables) {
+  } ¶
markhalliwell’s picture

FWIW, here's the markup for a checkbox with this patch:

<div class="form-type-checkbox form-item-cache form-item checkbox">
  <label class="option checkbox control-label" for="edit-cache">
    <input type="checkbox" id="edit-cache" name="cache" value="1" class="form-checkbox">
    Cache pages for anonymous users
  </label>
</div>

I noticed that the label has the "checkbox" class too, which causes the label to be indented inside the div. That should probably be removed (along with "option"?, idk).

Denes.Szabo’s picture

Rerolled the patch #22 against the latest 7.x-3.x.

Denes.Szabo’s picture

Status: Needs work » Needs review

Set the status to run the test against the patch.

Denes.Szabo’s picture

I fixed the label. removed option and the checkbox class too.

Denes.Szabo’s picture

I checked the option elements. There was the same error as above with checkbox elements. I removed these classes too.

markhalliwell’s picture

Status: Needs review » Needs work
+++ b/includes/form.inc
@@ -122,19 +124,9 @@ function bootstrap_form_element(&$variables) {
-      // Check if item exists in element whitelist
-      if (isset($element['#id']) && in_array($element['#id'], $whitelist)) {
-        $output .= ' ' . $prefix . $element['#children'] . $suffix . "\n";
-        $exclude_control = TRUE;
-      }
-      else {
-        $output = $exclude_control ? $output : $output.$control_wrapper;
-        $output .= ' ' . $prefix . $element['#children'] . $suffix . "\n";
-      }

If we're really going to remove usage of $whitelist, then we might as well remove the function too. Anyone see any complications with removing this? I have a sneaky suspicion that this will get added back in eventually though...

markhalliwell’s picture

  1. +++ b/includes/form.inc
    @@ -169,8 +155,10 @@ function bootstrap_form_element_label(&$variables) {
    +  $is_checkbox = $element['#type'] == 'checkbox';
    +  $is_radio = $element['#type'] == 'radio';
    

    Is there a way we could simplify this? Like:

    $skip_class = FALSE;
    if ('checkbox' === $element['#type'] || 'radio' === $element['#type']) {
      $skip_class = TRUE;
    }
    
  2. +++ b/includes/form.inc
    @@ -181,7 +169,7 @@ function bootstrap_form_element_label(&$variables) {
         $attributes['class'][] = 'option';
    

    We don't need the "option" class on labels anymore.

markhalliwell’s picture

Marked #2092665: Tableselect table types with '#multiple' => FALSE do not work as a dup of this issue. Multiples need to be considered.

Denes.Szabo’s picture

Status: Needs work » Needs review
FileSize
1.28 KB
7.11 KB

I fixed the label theme function according to your comments.

markhalliwell’s picture

Status: Needs review » Fixed

Thanks @Denes.Szabo!

Committed a1c07e8 to 7.x-3.x.
Committed 570ed73 to 7.x-3.x.

markhalliwell’s picture

Status: Fixed » Needs work

Still need to figure out if _bootstrap_element_whitelist() is needed since we're not actually using it anymore.

valkum’s picture

1. I'm not sure if we need a whitelist.

/**
 * Why whitelist an element?
 * The reason is to provide a list of elements we wish to exclude
 * from certain modifications made by the bootstrap theme which
 * break core functionality - e.g. ajax.
 */

Is this still the case? If not remove it.
I belive the change in bs for controls definition (moved from outer div to input) fixed the whitelist workaround.
edit-pass-pass1 and edit-pass-pass2 work fine without the whitelist workaround.

2. Other problem:

'checkboxes' is missing in $ignore_types array. Added patch.
For testing see account edit page.

markhalliwell’s picture

Thanks @valkum!

Committed #35:75f89e7 to 7.x-3.x.

Yeah, I noticed this too while working on #2094297: Fieldsets are unstyled, was about to fix it lol you beat me to it! FWIW though, I'm wondering if we should have more of an if/elseif switch in place instead of having to ignore all these types... searching an array to check for ignores make sense if it's only if the ones being applied still out-weight the ignores (which they don't now).

markhalliwell’s picture

@valkum, oh meant to also say: yes... let's just remove the $whitelist stuff (variables and function). We add back later if/when we run into issues.

valkum’s picture

Status: Needs work » Needs review
FileSize
1.75 KB

Heres the patch

markhalliwell’s picture

Status: Needs review » Fixed

Thanks @valkum!

Committed 08361d7 to 7.x-3.x.
Committed e15ad05 to 7.x-3.x.

tr33m4n’s picture

The issue I posted which was closed against this one #2092665: Tableselect table types with '#multiple' => FALSE do not work is still an issue, radio buttons do not display in tableselect

markhalliwell’s picture

I re-opened that issue.

markhalliwell’s picture

Version: 7.x-3.x-dev » 7.x-3.0-beta1
brian_ak’s picture

Great work on the BS3 beta everyone! wasn't entirely sure if I should post in this thread but I've noticed ['#field_prefix'] isn't outputting the proper html for BS3. While testing I've changed lines 124+125 of form.inc from:

$prefix = isset($element['#field_prefix']) ? '<span class="field-prefix">' . $element['#field_prefix'] . '</span> ' : '';
$suffix = isset($element['#field_suffix']) ? ' <span class="field-suffix">' . $element['#field_suffix'] . '</span>' : '';

to:

$prefix = isset($element['#field_prefix']) ? '<span class="input-group-addon">' . $element['#field_prefix'] . '</span> ' : '';
$suffix = isset($element['#field_suffix']) ? '<span class="input-group-addon">' . $element['#field_suffix'] . '</span>' : '';

which has worked for my needs.

markhalliwell’s picture

@brian_ak, thank you! Please open a new issue and create a patch.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.