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

Introduce clear event, and clear values before unselect event is triggered #5058

Closed
wants to merge 2 commits into from

Conversation

gronostajo
Copy link
Contributor

This pull request includes a

  • Bug fix
  • New feature
  • Translation

The following changes were made

@alexweissman
Copy link
Contributor

This looks good, and I appreciate that you added some tests as well!

Could you add a few comments to explain what your changes in allowClear.js are doing?

@gronostajo
Copy link
Contributor Author

Sure. If you need some comments in code, I can add them as well.

The Problem

Let's say you have a single select with allowClear: true. Clicking the clear button emitted select2:unselect first, then cleared the value, so checking for selected value in event handler returned something. This was especially problematic when building a generic single-or-multi-select component, because removing single items from multiple select uses the same select2:unselect event, but it worked the opposite way: item was removed first, then the event fired. This basically meant that you couldn't rely on select's value in unselect handlers.

The Fix, part 1

I've described that issue in #5049. This PR fixes it. The value before clearing is stored in a temporary variable and actual value is cleared. Then select2:unselecting and select2:unselect is emitted for each removed item (this part is left unchanged). Hovewer, if any of select2:unselecting events is cancelled, the value is restored from temporary variable and no more unselect events are emitted. If none of select2:unselecting events is cancelled, a change event is sent to select to update select2's DOM (this is where value was updated previously).

The Fix, part 2

In addition, I've added another event, select2:clear, and accompanying select2:clearing (related issue #4929). These differ from select2:unselect:

  • clear events are emitted only when clearing the select, not when removing single items from multiple select
  • clear events are always sent only once, no matter how many items are cleared
  • clear event happens before unselects
  • if select2:clearing is prevented, select2:clear and unselect event's won't be emitted at all

@alexweissman
Copy link
Contributor

Ok, I think I'm starting to understand. Basically, this makes the behavior of "clear selection in single-select" and "remove selection in multi-select" consistent. In that with your patch, in both cases the actual value of the control (as accessed via .val) is unset before the clear and unselect events are fired, right?

The data passed to these events though, is the same, right? It's the entire data properties object of the selection that was cleared/unselected?

@alexweissman
Copy link
Contributor

Also, what happens with allowClear in the multiselect case?

@gronostajo
Copy link
Contributor Author

Exactly, unselect is now consistent in both cases and clear works the same way. unselect data properties haven't changed and clear has an array of all cleared items. (unselect events contain single items)

To be honest, I haven't tested allowClear in multiselects, but I suppose it also presented the inconsistent behavior. There's just one piece of code that handles allowClear in both cases and it's actually written with multiselects in mind (single select is a special case of multiselect with single value). So there's no reason to suppose it works any different from single selects.

@alexweissman
Copy link
Contributor

Cool! I only ask because I know that there are some issues about allowClear and multi-select. See https://github.com/select2/select2/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20allowclear%20multi, in particular #3773 (which I think your clear event solves) and #3772.

Can you test this? This might be a good opportunity to deal with all of these allowClear issues in one blow 💪

@hoanghuan0701
Copy link

hoanghuan0701 commented Oct 5, 2017

For the issue with #3772. I've applied the new code changes of @gronostajo, but it does not work as expected. When you press backspace in multi selection, all selected items will be removed.

@alexweissman
Copy link
Contributor

@hoanghuan0701 thanks! But from what I can tell of #3772, this was a problem even without @gronostajo's patch? Can you confirm whether this patch is actually introducing a new bug?

@gronostajo
Copy link
Contributor Author

@alexweissman It seems that #3773 is resolved.

#3772 is present in my gist which demonstrates the problem I attempted to fix. Just add allowClear: true in the second select and it will start happening. And my patches indeed don't fix it as @hoanghuan0701 noted. Unfortunately I'm too busy to fix it anytime soon.

@alexweissman
Copy link
Contributor

That's fine, I just want to make sure that we're not introducing any new bugs.

@alexweissman alexweissman added this to the 4.0.6 milestone Oct 26, 2017
alexweissman added a commit that referenced this pull request Oct 26, 2017
@alexweissman
Copy link
Contributor

Merged into develop for 4.0.6 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants