openeuropa / oe_whitelabel

European Union Public License 1.2
2 stars 4 forks source link

Don't wipe existing settings in '#slim_select'. #235

Closed donquixote closed 9 months ago

donquixote commented 11 months ago

The oe_whitelabel integration of slim_select uses a '#process' callback on 'select' element type, to set $element['#slim_select'] = []. This will destroy any custom setting that might already exist on the element.

E.g. a custom module or theme could use hook_field_widget_complete_options_select_form_alter() to either add specific settings in $field_widget_complete_form['widget']['#slim_select'], or it could set $field_widget_complete_form['widget']['#slim_select'] = NULL to completely block the slim_select integration for this specific element. But this would be overwritten by _oe_whitelabel_process_element_select().

Two possible solutions:

  1. In _oe_whitelabel_process_element_select(), use $element += ['#slim_select' => []]; instead of $element['#slim_select'] = [];, to not overwrite existing values.
  2. In oe_whitelabel_element_info_alter(), use $info['select']['#slim_select'] = [];. This will be merged in FormBuilder::doBuildForm() where the element defaults are applied.

One question is what to do with the 'multi-select' class that is being added in _oe_whitelabel_process_element_select(). This is a bit weird: Currently this class is only added if slim_select module is enabled. And indeed, this class does have an effect on the styling of the multi select elements. But if this is the goal, why not target the classes that are added by slim-select library? Or add this class on client side, when we are sure that slim-select is being applied.

If we no longer need to add the class in php, and use option 2 from above, we can drop the callback.

donquixote commented 11 months ago

Btw the original goal was to remove the '_none' option if slim_select is active. See https://www.drupal.org/project/slim_select/issues/3393773

brummbar commented 10 months ago

Option 2 is not ok as we don't want to add ['#slim_select'] to all selects, only when it's a multivalue one. The custom class is to apply the behaviour defined in BCL. Could be refactored out, but that would break BC.

donquixote commented 10 months ago

Option 2 is not ok as we don't want to add ['#slim_select'] to all selects, only when it's a multivalue one.

Good point. So we need to keep the '#process'.

The custom class is to apply the behaviour defined in BCL. Could be refactored out, but that would break BC.

We could still add it on client side with js, to be sure it is only added if slim_select is active. But I think it is better to leave it as is until we experience real-world problems.


This leaves us with option one, which is a minor change to preserve existing config in the property.

brummbar commented 10 months ago

Since I'm handling something related to slim select, I'll fix that too. Will provide link soon.

donquixote commented 10 months ago

About removing the "None" option: For now we did that with custom code in the specific project. But I think it would make sense to always do this for widget with slim_select active.

brummbar commented 9 months ago

Fixed in #236 .