Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select2 default allow clear #14

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alexgreat70
Copy link

No description provided.

@tf
Copy link
Member

tf commented Jul 15, 2019

Hi @alexgreat70,

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

@alexgreat70
Copy link
Author

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.

@tf
Copy link
Member

tf commented Jul 24, 2019

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?

@alexgreat70
Copy link
Author

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

@tf
Copy link
Member

tf commented Jul 25, 2019

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 tf mentioned this pull request Dec 9, 2019
@tf tf marked this pull request as draft December 8, 2020 09:23
@alexgreat70 alexgreat70 marked this pull request as ready for review October 30, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants