-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix Infinite rendering in RAC ComboBox/Select with renderProps #4520
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 all commits
df8402e
2d734a6
fd6d3ed
58fc450
5fa9558
0a8fad2
afcdd4e
bebb9df
474a4ce
52eca14
05c68d3
fdee7e2
0fc5bcf
500ea69
c7869e4
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 |
|---|---|---|
|
|
@@ -13,14 +13,14 @@ import {AriaComboBoxProps, useComboBox, useFilter} from 'react-aria'; | |
| import {ButtonContext} from './Button'; | ||
| import {ComboBoxState, useComboBoxState} from 'react-stately'; | ||
| import {ContextValue, forwardRefType, Provider, RenderProps, slotCallbackSymbol, SlotProps, useContextProps, useRenderProps, useSlot} from './utils'; | ||
| import {filterDOMProps, useResizeObserver} from '@react-aria/utils'; | ||
| import {InputContext} from './Input'; | ||
| import {LabelContext} from './Label'; | ||
| import {ListBoxContext, ListBoxProps} from './ListBox'; | ||
| import {PopoverContext} from './Popover'; | ||
| import React, {createContext, ForwardedRef, forwardRef, useCallback, useRef, useState} from 'react'; | ||
| import React, {createContext, ForwardedRef, forwardRef, useCallback, useMemo, useRef, useState} from 'react'; | ||
| import {TextContext} from './Text'; | ||
| import {useCollection} from './Collection'; | ||
| import {useResizeObserver} from '@react-aria/utils'; | ||
|
|
||
| export interface ComboBoxProps<T extends object> extends Omit<AriaComboBoxProps<T>, 'children' | 'placeholder' | 'name' | 'label' | 'description' | 'errorMessage'>, RenderProps<ComboBoxState<T>>, SlotProps { | ||
| /** The filter function used to determine if a option should be included in the combo box list. */ | ||
|
|
@@ -41,11 +41,14 @@ function ComboBox<T extends object>(props: ComboBoxProps<T>, ref: ForwardedRef<H | |
| let state = useComboBoxState({ | ||
| defaultFilter: props.defaultFilter || contains, | ||
| ...props, | ||
| items: propsFromListBox ? (props.items ?? propsFromListBox.items) : [], | ||
| // If props.items isn't provided, rely on collection filtering (aka listbox.items is provided or defaultItems provided to Combobox) | ||
| items: props.items, | ||
|
Comment on lines
+44
to
+45
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For discussion: should we make users perform filtering on their ComboBox's ListBox themselves if they provide items to the ListBox? Or is that a case where we perform default filtering for them? See ComboBoxRenderPropsListBoxDynamic story for an example setup
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets discuss these things after grooming tomorrow |
||
| children: undefined, | ||
| collection | ||
| }); | ||
|
|
||
| // Only expose a subset of state to renderProps function to avoid infinite render loop | ||
| let renderPropsState = useMemo(() => ({isOpen: state.isOpen, isFocused: state.isFocused}), [state.isOpen, state.isFocused]); | ||
LFDanLu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| let buttonRef = useRef<HTMLButtonElement>(null); | ||
| let inputRef = useRef<HTMLInputElement>(null); | ||
| let listBoxRef = useRef<HTMLDivElement>(null); | ||
|
|
@@ -87,10 +90,13 @@ function ComboBox<T extends object>(props: ComboBoxProps<T>, ref: ForwardedRef<H | |
|
|
||
| let renderProps = useRenderProps({ | ||
| ...props, | ||
| values: state, | ||
| values: renderPropsState as ComboBoxState<T>, | ||
| defaultClassName: 'react-aria-ComboBox' | ||
| }); | ||
|
|
||
| let DOMProps = filterDOMProps(props); | ||
| delete DOMProps.id; | ||
|
|
||
| return ( | ||
| <Provider | ||
| values={[ | ||
|
|
@@ -114,7 +120,7 @@ function ComboBox<T extends object>(props: ComboBoxProps<T>, ref: ForwardedRef<H | |
| } | ||
| }] | ||
| ]}> | ||
| <div {...renderProps} ref={ref} slot={props.slot} /> | ||
| <div {...DOMProps} {...renderProps} ref={ref} slot={props.slot} /> | ||
| {portal} | ||
| </Provider> | ||
| ); | ||
|
|
||
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
as anybothers me, but I couldn't come up with a good way to mark the props as only accepting data attributes since we've been relying on how TS doesn't consider adata-*attribute to be an error if it isn't found in the element attributes type. Also I've forgone thedelete DOMProps.idwhen the props didn't share any values in common with the filterDOMProps type since we can reasonably expect onlydata-*attributes then.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.
do cells not support an id at all? I guess I'm a little confused as to what the issue with id is in this whole situation
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.
yeah, we don't accept ids on the CalendarCell at all
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.
if they are only
data-*and that is allowed onCalendarCellProps, you could create a type that would encompass it. maybe this would work?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.
Saving progress in
update_types_for_data_attributesbranch, the type changes are a bit extensive for this