-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(combobox): odd cursor behavior #17752
fix(combobox): odd cursor behavior #17752
Conversation
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17752 +/- ##
==========================================
- Coverage 79.56% 79.55% -0.01%
==========================================
Files 406 406
Lines 14019 14014 -5
Branches 4336 4334 -2
==========================================
- Hits 11154 11149 -5
Misses 2700 2700
Partials 165 165 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thanks for refining this @ariellalgilmore
useEffect(() => { | ||
if (inputRef.current) { | ||
inputRef.current.setSelectionRange(cursorPosition, cursorPosition); | ||
} | ||
}, [inputValue, cursorPosition]); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea what this was used for originally? It's wild that this whole useEffect was in there that not only wasn't needed, but actively caused a bug!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the cursorPosition came from the recent Autocomplete with Typeahead PR, but the odd cursor behavior was happening before that. When trying to fix the bug though, these additions seemed to probably help a weird cursor position when adding the autocomplete, so I realized fixing the original cursor bug also meant we could get rid of these new additions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM !!
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
a7d18f1
Closes #17222
Odd cursor behavior in combobox when typing "abc" then moving cursor between "a" and "b" the text was appear as "a1bc23" instead of "a123bc".
Screen.Recording.2024-10-15.at.9.56.24.AM.mov
resources: https://github.com/downshift-js/downshift/blob/master/src/hooks/useCombobox/README.md?plain=1#L581-L611
Changelog
Changed
Testing / Reviewing
Check combobox storybook