-
Notifications
You must be signed in to change notification settings - Fork 617
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
Fix "no choices to choose from"/"no results found" notice did not reliably trigger #1192
Conversation
Hi, I'm testing this branch It works better than the actual implementation, but there are still a few issues with "remote/dynamic" fields. First issue: "no choices" appearing when it should not appearI think it is related to some missing checks after const select = document.querySelector("#choices");
const choices = new Choices(select, { noChoicesText: 'Hello World!' } );
choices.passedElement.element.addEventListener(
'search',
function (e) {
choices.setChoices(
[{ value: 'alice', label: 'Alice' }, { value: 'bob', label: 'Bob' }],
'value',
'label',
true
)
}
) v11: https://jsfiddle.net/tagliala/rx8mtbz6/17/ v10: https://jsfiddle.net/tagliala/rx8mtbz6/18/ Second issue: "no choices" having priority over "no results found".It seems that "no choices" now have priority on "no results found". const select = document.querySelector("#choices");
const choices = new Choices(select, { noChoicesText: 'Hello World!' } );
choices.passedElement.element.addEventListener(
'search',
function (e) {
choices.setChoices(
[],
'value',
'label',
true
)
}
) This can be seen as a legit behaviour change, but I think that v10 worked better, showing "no results found" when a query was present even if choices were empty v11: https://jsfiddle.net/tagliala/rx8mtbz6/20/ v10: https://jsfiddle.net/tagliala/rx8mtbz6/22/ |
@tagliala It looks like I think the entire notice system needs a bit of a rework to avoid this ad-hoc matching and to be more centralized (basically making a notice reducer which listens for the relevant add/remove events. But for now the current code should work |
@Xon thanks, can you rebase this branch on master? I would like to take a look |
2775976
to
0b8d738
Compare
Should be rebased now |
No description provided.