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

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

Closed
tagliala opened this issue Aug 31, 2024 · 11 comments · Fixed by #1192
Closed

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

tagliala opened this issue Aug 31, 2024 · 11 comments · Fixed by #1192
Labels

Comments

@tagliala
Copy link
Contributor

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

  • Version: main

Desktop (please complete the following information):

  • OS: macOS
  • Browser Chrome / Firefox
  • Version 128.0.6613.114 / 129.0.2 (64 bit)
@tagliala tagliala added the bug label Aug 31, 2024
@tagliala
Copy link
Contributor Author

tagliala commented Aug 31, 2024

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
Copy link
Collaborator

Xon commented Aug 31, 2024

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

@Xon
Copy link
Collaborator

Xon commented Aug 31, 2024

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
Copy link
Contributor Author

tagliala commented Aug 31, 2024

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
Copy link
Collaborator

Xon commented Aug 31, 2024

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 Xon closed this as completed in #1192 Sep 4, 2024
@Xon
Copy link
Collaborator

Xon commented Sep 4, 2024

@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
Copy link
Contributor Author

tagliala commented Sep 4, 2024

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 Xon reopened this Sep 4, 2024
@Xon
Copy link
Collaborator

Xon commented Sep 4, 2024

This is encountering a few issues:

  • 'search' event is getting an empty query string to signal the search has stopped. Which happens on setChoices with replace is being called. This was a documented change.

  • clearChoices() is removing items.
    Items/choices as separate objects caused a lot of internal complexity, and it was documented in v11.0.0 around this design change. This sadly doesn't work well with using choices as autocomplete.

    I'll probably update clearChoices to just remove non-selected choices, this would match the behavior of early version.

  • clearChoices() is removing the "is searching flag", this is bug. But it was masking an issue inside setChoices()

  • setChoices() isn't setting the "rank" value on the Choice internally which results in the values being filtered out when in "search" mode. It previously sorted on non-unique score, but this could result in an unstable sort.

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
Copy link
Collaborator

Xon commented Sep 4, 2024

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
Copy link
Collaborator

Xon commented Sep 7, 2024

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

@Xon Xon closed this as completed Sep 7, 2024
@tagliala
Copy link
Contributor Author

tagliala commented Sep 7, 2024

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

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