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

Bug/auto complete change event not emitted #4290

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

timges
Copy link

@timges timges commented Feb 20, 2024

Closes #4289

Changelog

New

Changed

Updated the autocomplete-input component to now emit a change event, once a user input makes the input value matches an autocomplete suggestion. This was not the case before, because of the way the autocomplete functionality is implemented:

-> To render the suggestion into the input field, the value of the underlying input-ref is set to match the next suggestion (if available) after every user input. Thus when the actual input of the user matches the value which is already stored in the input-ref, no change is emitted.

I fixed that by intercepting the exact keystroke where this issue would occur and implemented some custom logic instead of the default behaviour. React brings a private property _valueTracker which is used to track changes and act as a trigger for reacts onChange. I utilized this to manually trigger a change event.

Removed

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

The implementation utilizes a private property which is supposd to be some react internal object. Using it might not be the most elegant solution.
Another approcah i thought of was to just intercept the keystroke as is but call the setInputValue directly, instead of using the _valueTracker

[...]
if(inputValue + event.key === autocompleteSuggestion (...)) {
  event.prevenDefault()
  setIputValue(inputRef.current.value)
}
[...]

User outcome would basically be the same, but there would be no changeEvent being emitted. This lead to my decision of going for the _valueTracker approach. Would love to hear youre thoughts on this 😊

Merge checklist

…nChange if the input matches the autocompleteSuggestion

the input value is set to match the suggestion after every input (that is how the autocomplete works). Thus no actual change happens when the incoming key makes the value match the suggestion, as they are already the equal. To fix this issue we have to fire the change event manually on that particular key down
@timges timges requested review from a team and joshblack February 20, 2024 13:52
Copy link

changeset-bot bot commented Feb 20, 2024

🦋 Changeset detected

Latest commit: 1fdb30c

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

This PR includes changesets to release 1 package
Name Type
@primer/react 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

@timges timges requested review from a team as code owners March 22, 2024 08:19
@timges timges requested a review from ericwbailey March 22, 2024 08:19
Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there! 👋 I'm so sorry about the delay on my end for this, thank you for being so patient 🙏

I think my only question was around the usage of the private property and if there was any way to get around that 😅 I think that's the only thing that stood out when taking a look.

I'll also include someone else to help me review the PR to help out. Just wanted to say thanks again for opening this PR!

@joshblack
Copy link
Member

cc @siddharthkp just wanted to include you for another set of 👀

@timges
Copy link
Author

timges commented Apr 22, 2024

Hi @joshblack,

No worries—the issue won’t run away 😄 And if it does, maybe that’s for the best.

It's been a few weeks, so I would need to refresh my memory on the context. From what I recall, there's no other way to resolve the issue without reworking the current autocompletion logic.

This stems from the input field's value being set to the autocomplete suggestion after each keystroke. Consequently, when the final character is typed, no change is detected because the value has already been updated. Using a private property is a straightforward method to force change detection, although it's clearly not the most elegant solution.

I’m not sure I’ll have the time to delve deeper into this at the moment, so for now, we might have to either stick with the provided solution or you could investigate further on your own. Sorry about that, but I’m currently quite busy with my main job 😅

Anyways, thank you so much for taking the time to review this PR! 🫶🏼 I’m excited to see this product evolve—even though I’m not even using it, lol. As someone working in the development of a corporate design system myself, it's inspiring to see what you created. Both design- and development wise. Kudos to you and your team!

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

Successfully merging this pull request may close these issues.

(Autocomplete) - Last typed character is lost on blur, when the value matches a suggestion
2 participants