-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix: Autocomplete context refactor #8695
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
Changes from 9 commits
c111744
24dd7cb
d8969a7
ae3cc84
3a81f31
db91b3a
7177106
dfb80f3
ec8066f
76107ff
cde0977
82cb364
18d3dbc
7cbf8a1
8bfe6d2
5a72a98
05aca0b
6cfdb09
213b1f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -10,29 +10,45 @@ | |||||
| * governing permissions and limitations under the License. | ||||||
| */ | ||||||
|
|
||||||
| import {AriaAutocompleteProps, CollectionOptions, useAutocomplete} from '@react-aria/autocomplete'; | ||||||
| import {AriaAutocompleteProps, useAutocomplete} from '@react-aria/autocomplete'; | ||||||
| import {AriaLabelingProps, DOMProps, FocusEvents, KeyboardEvents, Node, ValueBase} from '@react-types/shared'; | ||||||
| import {AriaTextFieldProps} from '@react-aria/textfield'; | ||||||
| import {AutocompleteState, useAutocompleteState} from '@react-stately/autocomplete'; | ||||||
| import {InputContext} from './Input'; | ||||||
| import {ContextValue, Provider, removeDataAttributes, SlotProps, SlottedContextValue, useSlottedContext} from './utils'; | ||||||
| import {mergeProps} from '@react-aria/utils'; | ||||||
| import {Node} from '@react-types/shared'; | ||||||
| import {Provider, removeDataAttributes, SlotProps, SlottedContextValue, useSlottedContext} from './utils'; | ||||||
| import React, {createContext, JSX, RefObject, useRef} from 'react'; | ||||||
| import {SearchFieldContext} from './SearchField'; | ||||||
| import {TextFieldContext} from './TextField'; | ||||||
|
|
||||||
| export interface AutocompleteProps<T> extends AriaAutocompleteProps<T>, SlotProps {} | ||||||
|
|
||||||
| interface InternalAutocompleteContextValue<T> { | ||||||
| // TODO: naming | ||||||
| // IMO I think this could also contain the props that useSelectableCollection takes (minus the selection options?) | ||||||
| interface CollectionContextValue<T> extends DOMProps, AriaLabelingProps { | ||||||
LFDanLu marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| filter?: (nodeTextValue: string, node: Node<T>) => boolean, | ||||||
| collectionProps: CollectionOptions, | ||||||
| collectionRef: RefObject<HTMLElement | null> | ||||||
| /** Whether the collection items should use virtual focus instead of being focused directly. */ | ||||||
| shouldUseVirtualFocus?: boolean, | ||||||
| /** Whether typeahead is disabled. */ | ||||||
| disallowTypeAhead?: boolean, | ||||||
| collectionRef?: RefObject<HTMLElement | null> | ||||||
| } | ||||||
|
|
||||||
| // TODO: naming | ||||||
| interface FieldInputContextValue<T = HTMLInputElement> extends | ||||||
|
||||||
| interface FieldInputContextValue<T = HTMLInputElement> extends | |
| interface AutocompleteInputContextValue<T = HTMLInputElement> extends |
this seems more reasonable, this is specific to the Autocomplete, whereas the CollectionContext is specific to the Collections. Unless we had something in mind for this context that wasn't Autocomplete?
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.
From my understanding, the eventual intent for these contexts is that they aren't specifically tied to Autocomplete and simply contain values/attributes/properties that a input might have. It may not even be an input though, could be a text area or some other control hence the generic default.
The future use case is still a bit hazy though tbh
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.
Sure, the one for collections is, but this input one does seem very tied to autocomplete
That or I'd argue both context should be defined somewhere else, that isn't inside the Autocomplete package
Outdated
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.
yes, move up
what is the ts error? Maybe we can fix it. though is useContextProps the correct thing to use anyways? Would we ever expect these to be passed as actual props? or only through the context?
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 error is as follows:
ideally we define the CollectionContext like so
export const CollectionContext = createContext<ContextValue<CollectionContextValue<any>, HTMLElement>>(null);
so that a consuming component like Menu can then call useContextProps like
[collectionProps, ref] = useContextProps(collectionProps, ref, CollectionContext);
in order to merge its own set of collectionProps and its own ref with the ones being provided by the Autocomplete via context. However, the ref type in Menu might be something like RefObject<HTMLDivElement | null> vs the CollectionContext's RefObject<HTMLElement | null> which then conflicts and typescript complains. The context's ref just needs to be something that we can dispatch events on and thus should be as generic as possible, but ContextValue doesn't take a generic it seems...
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 isn't necessary to use useContextProps, hence the current approach working in Menu/Listbox working due to casting the final ref merge as the desired ref type, but it would be nice to have that work IMO
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.
Updated with another attempt at the types
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.
Could use EventTarget? if it's just for dispatching events? or something else that they both inherit from?
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.
figured something out with @devongovett's help
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.
todo?
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 seems to work, but I'm realizing with the grid virtual focus work that there are a ton of configurations to test haha