fixed bug with undefined this.placeholder in allowClear.js and update… #4776
Conversation
|
Nice patch ! Can you add a test case ? 🎸 |
dist/js/select2.js
Outdated
| this.$element.val(this.placeholder.id).trigger('change'); | ||
| } else { | ||
| this.$element.val('').trigger('change'); | ||
| } |
There was a problem hiding this comment.
Please remove changes in dist: https://github.com/select2/select2/blob/master/CONTRIBUTING.md#submitting-a-pull-request
There was a problem hiding this comment.
I'll remove the change from distribution but wonder why is the distribution tracked when it is generated off other sources? Or why is something tracked that we don't want updated?
There was a problem hiding this comment.
We don't want it to create conflicts with other PRs, it'll be updated later. Looking forward to review the test case 🎸
jpic
left a comment
There was a problem hiding this comment.
Changes dist/ and needs a test
dist/js/select2.full.js
Outdated
| this.$element.val(this.placeholder.id).trigger('change'); | ||
| } else { | ||
| this.$element.val('').trigger('change'); | ||
| } |
There was a problem hiding this comment.
Change in dist/ better left off: https://github.com/select2/select2/blob/master/CONTRIBUTING.md#submitting-a-pull-request
There was a problem hiding this comment.
dist directory changes have been removed from the commit.
This bug and fix was mentioned in select2#3817
|
I removed the changes to the dist directory but creating a test for the bug in #3917 might take a while for me because I'd have to dig deeper into this bug and how the tests work while being pretty busy. I tried to trace the code and tests/selection/allowClear-tests.js seems like a good place for one but it could be a couple weeks. If that's too long, can someone else take my commit and finish off the test case? |
|
No problem, I'd rather wait two months now, than see a regression in two months and have you redo your patch ^^ |
|
The same error I met. |
|
@hxnan it isn't in master yet. You could make the requested test and submit a new pull request that is more merge-able to help this fix get into master. |
|
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. |
|
Hopefully someone can finish my fix for this so the pull request doesn't get closed by the github stale bot. It still fixes the bug even though there's no automated test on it. |
|
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. |
The issue is due to a select2 issue [0][1][2][3][4], a workaround that seem good enough for our context has been applied. Part of story #14190: get email notifications on pull requests - first stage [0] select2/select2#4323 [1] select2/select2#291 [2] select2/select2#4898 [3] select2/select2#3817 [4] select2/select2#4776 Change-Id: I2ae52204a41451b74ce40100588eac7ccca9823e
…d corresponding output files
This bug and fix was mentioned in #3817
This pull request includes a
The following changes were made
Like the commit says, I applied a fix mentioned in the comments of that issue. It fixed problems for me and I wanted to share the fix with any other select2 user.
-#3917
If this is related to an existing ticket, include a link to it as well.