-
Notifications
You must be signed in to change notification settings - Fork 526
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
refactor(Autocomplete): update tests and add safeTimeout to input #2545
Conversation
🦋 Changeset detectedLatest commit: d4a005e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
@@ -12,6 +12,7 @@ import {AutocompleteContext} from './AutocompleteContext' | |||
import TextInput from '../TextInput' | |||
import {useRefObjectAsForwardedRef} from '../hooks/useRefObjectAsForwardedRef' | |||
import {ComponentProps} from '../utils/types' | |||
import useSafeTimeout from '../hooks/useSafeTimeout' |
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 is a great hook. I didn't even realize we had it!
@@ -56,13 +58,13 @@ const AutocompleteInput = React.forwardRef( | |||
// HACK: wait a tick and check the focused element before hiding the autocomplete menu | |||
// this prevents the menu from hiding when the user is clicking an option in the Autoselect.Menu, | |||
// but still hides the menu when the user blurs the input by tabbing out or clicking somewhere else on the page | |||
setTimeout(() => { | |||
safeSetTimeout(() => { |
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.
It would be even better if we could refactor this setTimeout away entirely. InlineAutocomplete
managed to do it with a synthetic change event: https://github.com/primer/react/blob/main/src/drafts/InlineAutocomplete/InlineAutocomplete.tsx#L155
I don't think that should block the rest of these changes though.
}) | ||
}) | ||
|
||
describe('snapshots', () => { |
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.
Has this intentionally been moved outside of the "Autocomplete" describe
block?
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.
@mperrotti yeah, for a super odd reason too 🤔 Let me know if you have any ideas!
In order to get the snapshot tests and testing-library tests working together, I needed to split them up into a separate test suite. Otherwise, the mismatch act() warning would not go away. Let me know if there is a better approach when everything is in one test suite, I couldn't figure out a way to make it work 🤔
Context #2461
Update the tests for Autocomplete so that they no longer error out when running tests. These changes capture a variety of warnings, notably:
Note
In order to get the snapshot tests and testing-library tests working together, I needed to split them up into a separate test suite. Otherwise, the mismatch
act()
warning would not go away. Let me know if there is a better approach when everything is in one test suite, I couldn't figure out a way to make it work 🤔Changelog
New
Changed
AutocompleteInput
to use theuseSafeTimeout
hook instead ofsetTimeout
directlyAutocomplete
tests to no longer emit errors in consoleRemoved