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

fix: autocomplete for combobox #2028

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

okadurin
Copy link
Collaborator

@okadurin okadurin commented Jul 2, 2023

This pull request fixes a bug which I noticed in the lion-combobox component. First I will describe a bug and then there will be a section with explanation regarding the PR itself.

Bug description

Autocomplete feature is not being stable in multiple cases.

Case 1

Steps to reproduce

  • Navigate to https://lion-web.netlify.app/components/combobox/use-cases/#validation (Validation section)
  • Type m and wait until Mango autocompletion happens
  • Type one more symbol o and wait until autocompletion disappears
  • Type backspace 2 times to erase mo
  • Type m and wait until Mango autocompletion happens
  • Type one more symbol o and wait until autocompletion disappears
  • Hit Enter

Actual result

The autocomplete feature replaces mo to Mango

Expected result

Autocompletion should not happen since there are no words in the combobox list which starts from mo

A gif with example

combobox_mo

Case 2

Steps to reproduce

Actual result

The autocomplete feature replaces mak to Mango

Expected result

Autocompletion should not happen since there are no words in the combobox list which starts from mak

A gif with example

combobox_mak

Pull request description

To fix the issue the activeIndex of the lion-combobox should be set to -1 every time the unselection happens.
Note this is not the first iteration of the PR. I tried different approaches which might be more appropriate but those require more code updates and possible breaking changes which I don't want to bring without a discussion. F.e. there is a piece of existing code which sets activeIndex to -1 on every popup opening which means skip any active for all elements, but at the same time there is a unit test which tests setting active property programmatically before opening the popup. It seems this test doesn't make much sense for lion-combobox since any the active elements become non-active as soon as a popup opens. So supporting active state with closed combobox might be not a best idea.
If not having active elements with closed combobox is ok, then _handleAutocompletion might not be called on initial setup. Then only thing it does before opening a combobox popup is setting activeIndex (this can be ditched) and reset the model (this can be moved to another function).
Anyways we can discuss this and I can explain what I mean exactly in details.

@changeset-bot
Copy link

changeset-bot bot commented Jul 2, 2023

🦋 Changeset detected

Latest commit: 32077ac

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lion/ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@CLAassistant
Copy link

CLAassistant commented Jul 2, 2023

CLA assistant check
All committers have signed the CLA.

@gerjanvangeest gerjanvangeest added the enhancement New feature or request label Oct 4, 2023
@tlouisse tlouisse merged commit c459ded into ing-bank:master Nov 7, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants