-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add new Picker components #2375
Conversation
6983f63
to
5304553
Compare
3460b40
to
cf892ca
Compare
const useResetCache = (length: number) => { | ||
const resetCacheRef = useRef<VariableSizeList>(null); | ||
useEffect(() => { | ||
if (resetCacheRef.current) { | ||
resetCacheRef.current.resetAfterIndex(0, true); | ||
} | ||
}, [length]); | ||
return resetCacheRef; | ||
}; |
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.
What's useResetCache
doing for us? Is there an X
somewhere that this resets?
Why store the data outside of hooks like useState
?
We should probably add comments for all these decisions too.
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.
Something to do with virtualization. I just moved this code from Autocomplete
didn't look too deeply into it
>(() => { | ||
if (hasMultipleChoices) { | ||
if (value === undefined) { | ||
return defaultValue; | ||
} | ||
return [] as AutocompleteValue< | ||
OptionType, | ||
HasMultipleChoices, | ||
undefined, | ||
IsCustomValueAllowed | ||
>; | ||
} | ||
return value === undefined ? defaultValue : undefined; | ||
}, [defaultValue, hasMultipleChoices, value]); |
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.
I thought all this hasMultipleChoices
logic was in MUI. If it's something we have to check for all over the place, this is why I suggest we separate them into separate components.
Is this the only place it's relevant?
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.
I think this is the only place we do any type of checking. But, I also feel like this could be a simpler. Again, I just moved this code into the hook for reusability.
const { | ||
inputValueProp, | ||
isVirtualized, | ||
onChange, | ||
onInputChange, | ||
renderInput, | ||
t, | ||
valueProps, | ||
ListboxComponent, | ||
} = useAutocomplete<OptionType, HasMultipleChoices, IsCustomValueAllowed>({ |
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 shouldn't return t
right? If we need the translation function, we should grab it from that useTranslation
hook right?
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.
@KevinGhadyani-Okta I had mixed feelings about this. Felt a little weird to return it here but, I also felt like it might make sense to do it to keep from calling the hook again. I'm good either way
packages/odyssey-react-mui/src/labs/OdysseyPickers/ComposablePicker.tsx
Outdated
Show resolved
Hide resolved
packages/odyssey-react-mui/src/labs/OdysseyPickers/ComposablePicker.tsx
Outdated
Show resolved
Hide resolved
packages/odyssey-react-mui/src/labs/OdysseyPickers/PickerWithOptionAdornment.tsx
Show resolved
Hide resolved
packages/odyssey-react-mui/src/labs/OdysseyPickers/PickerWithOptionAdornment.tsx
Outdated
Show resolved
Hide resolved
12e54eb
to
8e72f70
Compare
I would like to see some unit tests, but it's overall fine after the changes we made the other day. |
792b979
to
b735520
Compare
DES-5646
Summary
Picker
andPickerWithOptionAdornment
Testing & Screenshots
Picker
PickerWithOptionAdornment
Small adornment
Large adornment