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

Files: 
CommentFileSizeAuthor
#38 bootstrap-remove_whitelist_function_and_calls-2084343-38.patch1.75 KBvalkum
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#35 bootstrap-missed_checkboxes_in_ignore_types-2084343-35.patch320 bytesvalkum
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch bootstrap-missed_checkboxes_in_ignore_types-2084343-35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#32 bootstrap-hook_form_element_cleanup_and_new_markup-2084343-32.patch7.11 KBDenes.Szabo
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#32 interdiff.txt1.28 KBDenes.Szabo
#28 bootstrap-hook_form_element_cleanup_and_new_markup-2084343-28.patch7.06 KBDenes.Szabo
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#28 interdiff.txt1.08 KBDenes.Szabo
#27 bootstrap-hook_form_element_cleanup_and_new_markup-2084343-26.patch7.02 KBDenes.Szabo
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#27 interdiff.txt1.1 KBDenes.Szabo
#25 bootstrap-hook_form_element_cleanup_and_new_markup-2084343-25.patch6.05 KBDenes.Szabo
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#22 bootstrap-hook_form_element_cleanup_and_new_markup-2084343-21.patch6.51 KBvalkum
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#20 bootstrap-add-preprocessor-for-element-altering-2084343-20.patch1.02 KBvalkum
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#18 bootstrap-add-preprocessor-for-element-altering-2084343-18.diff1.02 KBvalkum
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#17 bootstrap-add-preprocessor-for-element-altering-2084343-17.diff1015 bytesvalkum
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#9 form-bootstrap3.png14.77 KBDenes.Szabo
#8 bootstrap-3-updated_form_preprocessing-2084343-8.patch9.2 KBheylookalive
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#3 bootstrap-3-updated_form_preprocessing-2084343-3.patch19.2 KBheylookalive
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]
#1 bootstrap-added-some-theme-changes-for-bs3-2084343-1.patch12.87 KBvalkum
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch bootstrap-added-some-theme-changes-for-bs3-2084343-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs work
StatusFileSize
new12.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch bootstrap-added-some-theme-changes-for-bs3-2084343-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Works fine for me.

Status:Needs work» Needs review
StatusFileSize
new19.2 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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

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?

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.

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.

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.

Status:Needs work» Needs review
StatusFileSize
new9.2 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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.

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new14.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.

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!

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

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?

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".

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".

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.

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

StatusFileSize
new1015 bytes
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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.

StatusFileSize
new1.02 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.02 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Ok here with changes from #19

Status:Needs review» Fixed

Thanks @valkum!

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

Status:Fixed» Needs review
StatusFileSize
new6.51 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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.

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) {
+  } ¶

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).

StatusFileSize
new6.05 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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

Status:Needs work» Needs review

Set the status to run the test against the patch.

StatusFileSize
new1.1 KB
new7.02 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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

StatusFileSize
new1.08 KB
new7.06 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

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

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...

  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.

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

Status:Needs work» Needs review
StatusFileSize
new1.28 KB
new7.11 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

I fixed the label theme function according to your comments.

Status:Needs review» Fixed

Thanks @Denes.Szabo!

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

Status:Fixed» Needs work

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

StatusFileSize
new320 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch bootstrap-missed_checkboxes_in_ignore_types-2084343-35.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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).

@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.

Status:Needs work» Needs review
StatusFileSize
new1.75 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

Heres the patch

Status:Needs review» Fixed

Thanks @valkum!

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

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

I re-opened that issue.

Version:7.x-3.x-dev» 7.x-3.0-beta1

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.

@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.

Issue summary:View changes

Updated issue summary.