Resolved issue #4511 to reposition open dropdown when a selection is made #4601
Conversation
There was a problem hiding this comment.
I've left a few in-line comments about things which you may want to look into before we can accept this pull request into Select2.
Additionally, it would be helpful if we could get a test case that verifies that this issue has been fixed, so we don't need to worry about it breaking in the future. The test case should fail before this commit is applied, and pass afterwards. You can find some existing positioning tests in the repository to give you a start.
If you can't get a working test case, that's not as much of an issue, it's just something we prefer.
| @@ -32,6 +32,15 @@ define([ | |||
| self._resizeDropdown(); | |||
| }); | |||
| } | |||
|
|
|||
| function repositionOnSelectionChange() { | |||
There was a problem hiding this comment.
The tabbing appears to be off here, as it looks like you are using a combination of spaces and tabs.
There was a problem hiding this comment.
Ah, yes - apologies. It looks like your convention is a space indented file, so I'll modify.
| @@ -32,6 +32,15 @@ define([ | |||
| self._resizeDropdown(); | |||
| }); | |||
| } | |||
|
|
|||
| function repositionOnSelectionChange() { | |||
| if (!this.options.get('closeOnSelect')) { | |||
There was a problem hiding this comment.
What was the reasoning behind the special case here?
There was a problem hiding this comment.
You may only want to reposition the dropdown if you can detect that the dropdown is still open. I think that may have been what you were going for here, but you can probably solve the issue by looking at the dropdown state instead of checking for a specific option.
There was a problem hiding this comment.
I was originally trying to only reposition if I knew the dropdown wasn't about to be closed due to the closeOnSelect option. I had originally incorrectly assumed the container.isOpen might have given me the wrong value depending on the order of the decorator chain, however, it does look like this check works more generically. I'll modify and commit.
|
@kevin-brown Thanks for the quick review of this PR. I looked over the positioning-tests.js - it would take me a while to digest it all and write an appropriate unit test. It looks like I would need to try to combine the approaches in positioning-tests and possibly selectOnClose-tests.js to render the dropdown with a small width, set closeOnSelect to false, add multiple options to cause the selection element to wrap and re-check the position of the dropdown? I know the value of making sure the tests are in place. I don't know that I can post a test quickly. I hate to ask, but would you be able to help write one for this change? Thanks, |
devmonkey22
changed the title
|
Can we please get this Pull Request Merged. This was causing a huge problem with our application when we upgraded from 3.x to 4.x, I had to apply this patch to a local copy of select2, but it worked beautifully and solved the problem. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request includes a
The following changes were made:
If this is related to an existing ticket, include a link to it as well.
#4511