activeadmin-searchable_select icon indicating copy to clipboard operation
activeadmin-searchable_select copied to clipboard

Select2 default allow clear

Open alexgreat70 opened this issue 6 years ago • 5 comments

alexgreat70 avatar Jul 09 '19 18:07 alexgreat70

Hi @alexgreat70,

thank you for your contribution. Can you explain the reasoning behind this change?

tf avatar Jul 15 '19 11:07 tf

Hi @alexgreat70,

thank you for your contribution. Can you explain the reasoning behind this change?

This change adds the ability to reset the default field values, which is very convenient.

alexgreat70 avatar Jul 16 '19 15:07 alexgreat70

I tested it and agree that it makes a lot of sense. In the form input case it can be a bit confusing, though, if the underlying select does not have a blank option: There should not be a way to empty the select, only to choose another option. On the other hand, if a blank value is present, the new behavior is better since before Select2 did not provide a way to reset the select. Still, the label of the blank option is not displayed.

I'd suggest to

  • change the implementation such that placeholder and allowClear are only set when include_blank is true for the select input. This info could be passed using a data attribute or by looking for the required attribute that Formtastic appears to be setting on the surrounding div.

  • as a bonus, consider looking for the option with blank value and using its text as placeholder. This would make sure the blank option label is displayed.

What do you think?

tf avatar Jul 24 '19 17:07 tf

I tested it and agree that it makes a lot of sense. In the form input case it can be a bit confusing, though, if the underlying select does not have a blank option: There should not be a way to empty the select, only to choose another option. On the other hand, if a blank value is present, the new behavior is better since before Select2 did not provide a way to reset the select. Still, the label of the blank option is not displayed.

I'd suggest to

  • change the implementation such that placeholder and allowClear are only set when include_blank is true for the select input. This info could be passed using a data attribute or by looking for the required attribute that Formtastic appears to be setting on the surrounding div.
  • as a bonus, consider looking for the option with blank value and using its text as placeholder. This would make sure the blank option label is displayed.

What do you think?

I think this should not be done, placeholder can be specified if it is really necessary, we have been using your gem for a month already and only added feature works for us, it works through fork, we are waiting for confirmation of the merge so that we can use your repository directly in Gemfile

alexgreat70 avatar Jul 25 '19 05:07 alexgreat70

Since the change makes matters worse in the case of include_blank: false, I will not merge the current version. I'll leave the PR open in case somebody else decides to pick it up.

tf avatar Jul 25 '19 06:07 tf