Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comments
Comment #1
valkum CreditAttribution: valkum commentedi 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.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedWorks fine for me.
Comment #3
heylookalive CreditAttribution: heylookalive commentedFollowing 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
Comment #4
markhalliwellDon't include the overrides, this will be handed in another issue.
Whitespace at EOL.
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?
Comment #5
heylookalive CreditAttribution: heylookalive commentedHey 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.
Comment #6
markhalliwellLooking 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.Comment #7
heylookalive CreditAttribution: heylookalive commentedAh, 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.
Comment #8
heylookalive CreditAttribution: heylookalive commentedWhen 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.
Comment #9
Denes.Szabo CreditAttribution: Denes.Szabo commentedYour 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.
Comment #10
heylookalive CreditAttribution: heylookalive commentedHi @denes, yep. As @mike said we'll tackle this in #2084331: Update and recompile the overrides.less/css file. Thanks for the patch review!
Comment #11
Denes.Szabo CreditAttribution: Denes.Szabo commentedOf course. I added a patch that issue too (#2084331).
Comment #12
markhalliwell@heylookalive, re:
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?
Comment #13
heylookalive CreditAttribution: heylookalive commentedMorning, 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".
Comment #14
heylookalive CreditAttribution: heylookalive commentedMorning, 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".
Comment #15
heylookalive CreditAttribution: heylookalive commentedDouble 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.
Comment #16
markhalliwell@heylookalive, yes drupal_alter() allows themes to alter data.
Comment #17
valkum CreditAttribution: valkum commentedI 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.
Comment #18
valkum CreditAttribution: valkum commentedI'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.
Comment #19
markhalliwellOver all very good! A few tiny nitpicks though:
We could probably just do:
!empty($element['#input']
here.Typos. Replace with:
Don't inject the "form-control" class on these input types.
Rename the
$types
variable to$ignore_types
to have more semantic meaning.Comment #20
valkum CreditAttribution: valkum commentedOk here with changes from #19
Comment #21
markhalliwellThanks @valkum!
Committed 9f38f98 to 7.x-3.x.
Comment #22
valkum CreditAttribution: valkum commentedI'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.
Comment #23
markhalliwellHmm, interesting. I'll need to manually review this. Quick review (using Dreditor btw) detected whitespace issues:
Comment #24
markhalliwellFWIW, here's the markup for a checkbox with this patch:
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).
Comment #25
Denes.Szabo CreditAttribution: Denes.Szabo commentedRerolled the patch #22 against the latest 7.x-3.x.
Comment #26
Denes.Szabo CreditAttribution: Denes.Szabo commentedSet the status to run the test against the patch.
Comment #27
Denes.Szabo CreditAttribution: Denes.Szabo commentedI fixed the label. removed option and the checkbox class too.
Comment #28
Denes.Szabo CreditAttribution: Denes.Szabo commentedI checked the option elements. There was the same error as above with checkbox elements. I removed these classes too.
Comment #29
markhalliwellIf 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...
Comment #30
markhalliwellIs there a way we could simplify this? Like:
We don't need the "option" class on labels anymore.
Comment #31
markhalliwellMarked #2092665: Tableselect table types with '#multiple' => FALSE do not work as a dup of this issue. Multiples need to be considered.
Comment #32
Denes.Szabo CreditAttribution: Denes.Szabo commentedI fixed the label theme function according to your comments.
Comment #33
markhalliwellThanks @Denes.Szabo!
Committed a1c07e8 to 7.x-3.x.
Committed 570ed73 to 7.x-3.x.
Comment #34
markhalliwellStill need to figure out if
_bootstrap_element_whitelist()
is needed since we're not actually using it anymore.Comment #35
valkum CreditAttribution: valkum commented1. I'm not sure if we need a whitelist.
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.
Comment #36
markhalliwellThanks @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).
Comment #37
markhalliwell@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.
Comment #38
valkum CreditAttribution: valkum commentedHeres the patch
Comment #39
markhalliwellThanks @valkum!
Committed 08361d7 to 7.x-3.x.
Committed e15ad05 to 7.x-3.x.
Comment #40
tr33m4n CreditAttribution: tr33m4n commentedThe 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
Comment #41
markhalliwellI re-opened that issue.
Comment #42
markhalliwellComment #43
brian_ak CreditAttribution: brian_ak commentedGreat 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:
to:
which has worked for my needs.
Comment #44
markhalliwell@brian_ak, thank you! Please open a new issue and create a patch.
Comment #45.0
(not verified) CreditAttribution: commentedUpdated issue summary.