react icon indicating copy to clipboard operation
react copied to clipboard

Bug/auto complete change event not emitted

Open timges opened this issue 1 year ago • 3 comments

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

  • [x] 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

  • [x] Added/updated tests
  • [ ] Added/updated documentation
  • [ ] Added/updated previews (Storybook)
  • [x] Changes are SSR compatible
  • [x] Tested in Chrome
  • [x] Tested in Firefox
  • [x] Tested in Safari
  • [x] Tested in Edge
  • [ ] (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

timges avatar Feb 20 '24 13:02 timges

🦋 Changeset detected

Latest commit: 1fdb30c1524917015ded046bb5c7535d78fbdbe0

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

changeset-bot[bot] avatar Feb 20 '24 13:02 changeset-bot[bot]

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

joshblack avatar Apr 12 '24 19:04 joshblack

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!

timges avatar Apr 22 '24 08:04 timges

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

github-actions[bot] avatar Jul 04 '24 17:07 github-actions[bot]