Choices-js / Choices

A vanilla JS customisable select box/text input plugin ⚡️
https://choices-js.github.io/Choices/
MIT License
6.13k stars 605 forks source link

[v11] `main` branch: Regression in "no choices text" when clearing search field #1191

Closed tagliala closed 1 week ago

tagliala commented 2 weeks ago

Describe the bug Hello, this is a follow up of #1185

To Reproduce Steps to reproduce the behavior:

  1. Go to https://jsfiddle.net/tagliala/rx8mtbz6/13 Based on Choices 11 (main)
  2. Click on the input field
  3. Insert a letter
  4. Delete the letter
  5. noChoicesText does not Appear

On v10.2, the text appears: https://jsfiddle.net/tagliala/rx8mtbz6/9

Expected behavior See "Hello World"

Screenshots

v10

image

v11

image

Choices version and bundle

Desktop (please complete the following information):

tagliala commented 2 weeks ago

I think that there are other regression with the search functionality, related to how noChoicesText and noResultsText are being used, but I'm not confident to share the results at the moment, I prefer to provide reproducible test cases (and maybe the issues are related, so maybe fixing this one will fix others too)

Xon commented 2 weeks ago

Thanks for identifying this, it looks like this is going to need more work on when noChoicesText or noResultsText is displayed

Xon commented 2 weeks ago

I've added an e2e test which reproduces the issue, but it will be in a day or so before I can dig into the notice display logic for this

tagliala commented 2 weeks ago

No problems, thanks again for your support. I'm not a JavaScript developer and I'm not familiar with choices.js internals so it would require more time on my side to try and submit a PR.

Xon commented 2 weeks ago

PR #1192 should work better, it cleaned up some possible issues with no results vs no choices. I haven't had time to fully test it and see if any of the other end-to-end tests need updating to verify the behavior is working properly.

Xon commented 1 week ago

@tagliala main should have the most recent rounds of fixes. If nothing else is reported I plan to release v11.0.2 at the end of the week

tagliala commented 1 week ago

Hi @Xon, I've tested main, works almost like v10, there one more thing with a slightly more complex scenario

https://jsfiddle.net/tagliala/rx8mtbz6/40/

  1. Enter a letter
  2. Delete the letter (to empty the field)

Expected

See "No Choices"

Actual

Empty

image
const select = document.querySelector("#choices");
const choices = new Choices(select, {
  noChoicesText: 'No Choices!'
});

choices.passedElement.element.addEventListener(
  'search',
  function(e) {
    const query = e.detail.value

    if (query.length < 2) {
      choices.clearChoices()
      return
    }

    let result = []
    if (query.slice(0, 2) === 'fo') {
      result = [{
        value: 'found',
        label: 'Found'
      }]
    }

    choices.setChoices(
      result,
      'value',
      'label',
      true
    )
  }
)
Xon commented 1 week ago

This is encountering a few issues:

I'm not sure what the best solution is for using choices as a autocomplete widget with these constrains, I'll need to consider possible solutions.

Xon commented 1 week ago

I've adjusted setChoices/clearChoices in main to hopefully be less surprising, which should work better.

There are still some minor behavior differences (how selected items/choices show up), but I think it is acceptable without major changes.

Xon commented 1 week ago

v11.0.2 has a few more bugfixes, and should be in a much better state. There is now an autocomplete e2e test based on your snippet should should catch regressions

tagliala commented 1 week ago

Thanks, I've tested the main branch and it now works better

I've switched to searchFloor instead of early returning from search when there are less than two chars, it is not exactly the same behavior as before but works better overall