Extending on the changes made in #2051119: Field's minimum width should be passed through options of the chosen instance, I have introduced some extra configuration for the minimum width setting that allows users to have more control over how they want their Chosen field to appear.

For my particular use case, I needed to have it have a percentage-based width to support use in a responsive layout. Because of the inline style, this was a pain to do in custom css (required an !important flag which I just hate doing), so adjusted it here instead.

The changes just add an extra configuration option for minimum width units:
New width configuration settings

I had started down a track of removing the inline style altogether which I believe would be more theme-friendly than this, but understand that there are a lot of users who don't want to or can't tinker with the theme css themselves.

I also considered just lumping it all in the one field, but I think this way is a bit more noob safe.

It still defaults to 200px so nothing changes if you hadn't configured it yet.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marblegravy’s picture

Status: Active » Needs review
FileSize
4.47 KB
marblegravy’s picture

Retrospectively, perhaps references to a minimum width should also be replaced with just 'width.' After all, that's what we're setting - the actual width. There's nothing much minimum about it!

Thoughts?

Hydra’s picture

Status: Needs review » Needs work

Generally I think this would be a good improvement! Wording and usability of course is an issue we also address in #2064325: Revise configuration page, perhaps we can put our thoughts here together.
Some little issues with your patch:

  1. +++ b/chosen.admin.inc
    @@ -25,16 +25,29 @@ function chosen_admin_settings($form, &$form_state) {
    -  $form['chosen_minimum_width'] = array(
    +  $form['chosen_width_settings'] = array(
    

    Renaming will cause the lost of previously saved data. We need an update hook to transfer this data to the new variable.

  2. +++ b/chosen.admin.inc
    @@ -25,16 +25,29 @@ function chosen_admin_settings($form, &$form_state) {
         '#required' => TRUE,
    ...
    +    '#options' => array('px' => 'px', 'em' => 'em', '%' => '%'),
    
    +++ b/chosen.js
    @@ -31,10 +32,10 @@
    \ No newline at end of file
    

    Missing new line of the end of the javascript file.

Besides this, the admin_settings_form is storing the chosen_width_settings in a variable, which needs to be deleted in chosen.install chosen_uninstall() (and of course the chosen_minimum_width one will be obsolete).

marblegravy’s picture

Assigned: Unassigned » marblegravy

Thanks for the review - I'll try and have another crack at it.

Harlor’s picture

Component: Code » Miscellaneous
FileSize
3.82 KB

Well I converted the patch for the new dev version. I think there is an issue with the units in the chosen.js file.

+++ b/chosen.js
@@ -30,7 +31,7 @@
-              width: (($(this).width() < minWidth) ? minWidth : $(this).width()) + 'px'
+              width: (($(this).width() < minWidth) ? minWidth : $(this).width()) + minWidthUnits

@@ -39,7 +40,7 @@
-          width: (($(this).width() < minWidth) ? minWidth : $(this).width()) + 'px'
+          width: (($(this).width() < minWidth) ? minWidth : $(this).width()) + minWidthUnits

This wont work for other units!

$(this).width() is always in px, minWdth is not.

Well I thing there is no possibility to convert tehse units... maybe we have to accept that for other units the setting is not the min setting but the absolute setting.

Hydra’s picture

Status: Needs work » Postponed

Well patch looks good so far to me, except the unit thingy :/
One possible solution would be to seperate px from other units in the comparison. So if unit == px, we make the minWidth check. If not, the value will be used as fix width for all select fields.

I would like to postpone this till the conversion to plugins has been made. Perhaps while creating a generic way to implement plugin-based options, we find a practical solution. #2076813: Move library options (configuration) to library plugin

MrPaulDriver’s picture

Issue summary: View changes

Any update on this

skorzh’s picture

Status: Postponed » Needs review
FileSize
2.83 KB

Thank you guys for your patchs, but it doesn't work with current version of module.
I created a new patch for this problem, I didn't test it for all possible issues, but seems it works for me well.

gmclelland’s picture

Status: Needs review » Reviewed & tested by the community

Thanks. Patch in #8 works for me, but it did have some whitespace errors.

Glenns-MacBook-Pro:chosen glenn$ git apply -v chosen-responsive_width-2075579-8.patch
chosen-responsive_width-2075579-8.patch:9: trailing whitespace.

Checking patch chosen.admin.inc...
Checking patch chosen.js...
Checking patch chosen.module...
Applied patch chosen.admin.inc cleanly.
Applied patch chosen.js cleanly.
Applied patch chosen.module cleanly.
warning: 1 line adds whitespace errors.

The last submitted patch, 1: chosen-add-additional-minwidth-settings-2075579-1.patch, failed testing.

The last submitted patch, 5: chosen.code_.2075579-5.patch, failed testing.

dddbbb’s picture

Status: Reviewed & tested by the community » Needs work

While the patch in #8 works in that it does what it sets out to do, I still don't think that this is the right solution: We're still left with an inline width style on the chosen-container div and overriding this from the theme still requires the use of !important in stylesheets (not good).

I think that we should be removing the inline style altogether or at least providing a config option for this.

dddbbb’s picture

Looks like this kind of approach is in the works in the Chosen.js library: https://github.com/harvesthq/chosen/pull/2091

joelpittet’s picture

I think that we should be removing the inline style altogether or at least providing a config option for this.

@danbohea, I agree. Maybe removing the requirement of this and setting it to empty would suffice with the latest chosen library?

joelpittet’s picture

Made a patch for that over here to not disrupt this one #2790897: Let the width be flexible

jstoller’s picture

Status: Needs work » Needs review
FileSize
4.39 KB

I rerolled #8 to work with the latest 7.x-2.x-dev release, making the following changes:

  • Some UI and text changes in the admin interface to improve UX
  • Variable names have been changed to chosen_width and chosen_width_units
  • Added validation to chosen_width, to make sure it's numeric
  • The width units are now specified in a textfield, with "px" as the default, allowing complete flexibility
  • Added an update function to retain existing configuration, if there is one set
  • Updated chosen_uninstall() to delete new variables