Support from Acquia helps fund testing for Drupal Acquia logo

Comments

el7cosmos’s picture

Status: Active » Needs review
FileSize
807 bytes
markhalliwell’s picture

Status: Needs review » Needs work
+++ b/includes/theme.inc
@@ -126,7 +126,7 @@ function theme_bootstrap_links($variables) {
-  $type_class = '';
+  $type_class = 'btn-default';
...
   $variables['attributes']['class'][] = 'btn-group';

Setting $type_class here will potentially get overwritten on ln 135.

You should add similarly like the btn-group.

el7cosmos’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

Yeah it meant to be overridden by other modifier.

But problem is, I get repetitive btn-default class in theme_button since it loops each array element to check button value, so I decide to assign default class to be overridden as the above case.

markhalliwell’s picture

Status: Needs review » Needs work

I'm just not too thrilled with this approach TBCH. It needs work. Feel free to completely refactor it if needed (instead of trying to work within the confines of the existing code).

valkum’s picture

Status: Needs work » Needs review
FileSize
1.39 KB

Hey there, i took patch #3 and restructured the code a little bit. Also i added a var for theme_button called type similar to btn_dropdown

It works so far.

Anonymous’s picture

Works for me too. Looks good.

oriol_e9g’s picture

Status: Needs review » Needs work

The last submitted patch, bootstrap-add-default-class-button-and-var-codingstd.patch, failed testing.

heylookalive’s picture

Status: Needs work » Needs review
FileSize
1.36 KB

How about this, if we don't have any decent classes on a button at theme_button stage, throw in the default. Much simpler than other methods.

Also have got the bootstrap dropdown change present in other patches, I don't have eyes on that as an issue though so am not sure if that's correct or not.

markhalliwell’s picture

Status: Needs review » Needs work
  1. +++ b/includes/form.inc
    @@ -260,6 +260,22 @@ function bootstrap_button($variables) {
       if (!empty($element['#attributes']['disabled'])) {
         $element['#attributes']['class'][] = 'form-button-disabled';
    

    Is this still the correct class?

  2. +++ b/includes/theme.inc
    @@ -134,6 +134,9 @@ function theme_bootstrap_btn_dropdown($variables) {
       if (isset($variables['type'])) {
         $type_class = ' btn-'. $variables['type'];
       }
    +  else {
    +    $type_class = ' btn-default';
    +  }
    

    I'd really not have to do this. Is there a reason why we can't just throw out all the old crap?

Anonymous’s picture

.btn must also use .btn-default to get the "default" button.

http://getbootstrap.com/getting-started/#migration

.btn .btn-default

heylookalive’s picture

@mark disabled in bS3 works off of the button element disabled attribute which by the looks of that code should be fine, but will have an extra class on it whilst not needed could be helpful to people for theming.

On 2, can't say that I use this but for backwards compatibility should stick with it I guess (as a maintainer you can make the call to bin it, i have no preference personally), but as said I haven't paid any attention to this chunk of functionality/theming. The change makes logical sense in providing the default class. @morningtime, will need to check how this works but yep should have the ".btn" class on it as per docs.

markhalliwell’s picture

but will have an extra class on it whilst not needed could be helpful to people for theming.

.btn[disabled] will work just fine. Having classes like this is superfluous. I know it wasn't added in this patch, but I'd like to just remove it here. My goal with the migration from 2.x to 3.x wasn't just a simple conversion, but also cleanup of the cruft that has been added over time through the various project merges.

For #10.2, I say we either remove this or change it from .btn-$type to .form-$type. I don't want to confuse the .btn prefix with Drupal specifics.

valkum’s picture

Isn't the $variable type param meant to be set by someone who's using this hook? To set which style the dropdown button is shown?

markhalliwell’s picture

@valkum, no the dropdown buttons have different classes and markup altogether (see: http://getbootstrap.com/components/#btn-dropdowns). The $variables['#type'] is the type of element in the FAPI. It has nothing to do with Bootstrap.

markhalliwell’s picture

@heylookalive in #12, in response to backwards compatibility: The migration from 2.x to 3.x is not meant to be "compatible" there is too much that has changed. I'm ok with "breaking" BC here.

valkum’s picture

It is 'type' not '#type'
From our hook_theme definition

...
'bootstrap_btn_dropdown' => array(
      'variables' => array(
        'links' => array(),
        'attributes' => array(),
        'type' => NULL
      ),
    ),
...

I thought someone who's using the bootstrap_btn_dropdown theme in his subtheme or module can define which option is set as class for the buttin (primary, alert, etc.)

markhalliwell’s picture

Ah... good catch. I forget about that theme hook sometimes. Thanks! Yeah, I guess leave it in for now, make sure it works though. Some 'type' classes might need updating.

markhalliwell’s picture

Status: Needs work » Fixed

Thanks @heylookalive!

Committed 24dd443 to 7.x-3.x.
Committed d4973fc to 7.x-3.x.

markhalliwell’s picture

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

Status: Fixed » Closed (fixed)

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