fixing combobox to use useFormProps and related component refactors#2351
Conversation
ktabors
requested review from
dannify,
devongovett and
snowystinger
as code owners
|
Build successful! 🎉 |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
The other components look good to me, just one thing w/ mobile combobox
|
Build successful! 🎉 |
|
Reviewed the chromatic and the only updates are combobox side label and necessity indicator working. https://www.chromatic.com/build?appId=5f0dd5ad2b5fc10022a2e320&number=136 |
|
Build successful! 🎉 |
| } | ||
|
|
||
| function TextFieldBase(props: TextFieldBaseProps, ref: Ref<TextFieldRef>) { | ||
| props = useProviderProps(props); |
There was a problem hiding this comment.
how'd we remove useProviderProps from this one? but not from SearchWithin?
There was a problem hiding this comment.
useProviderProps is useful for a component that is being rendered directly like SearchWithin. If it is a child like TextFieldBase which other components use as a child, this call doesn't do anything. And useProviderProps can't exist here and not in the parent. useProviderProps must be in the parent. It we had Storybook tests of TextFieldBase we would need this useProviderProps call.
There was a problem hiding this comment.
IMO I think we are ok pulling useProviderProps from TextFieldBase, feels like calling that hook is something each parent component that uses TextFieldBase should handle (e.g. if a component needs to override one of the provider props).
There was a problem hiding this comment.
On second thought, useProviderProps favors the provided props over the props from the Provider so perhaps it would make sense to keep useProviderProps in TextFieldBase and remove it from the parent components that use it?
There was a problem hiding this comment.
Just to remind myself, I move useProviderProps from TextField to TextFieldBase. I goto the form stories, things look good until I get to isDsiabled, TextField is not disabled. Same in the provider stories. This might be a bug with isDisabled.
Looking at SearchWithin it seems like it shouldn't need useProviderProps, but when I remove it isDisabled breaks (which is weird, because I'd expect SearchField to pick it up) and the required indicator stops working.
I think there is a bug. I'm debugging the code and found that calling useProviderProps({}) in SearchField returns
isDisabled: true
isEmphasized: undefined
isQuiet: undefined
isReadOnly: undefined
isRequired: undefined
validationState: undefined
but calling the props after props = useProviderProps(props); still have isDsiabled as undefined.
UNSAFE_className: "searchwithin_spectrum-SearchWithin-searchfield_9a8c2"
aria-label: undefined
aria-labelledby: "react-aria489618031-2628"
id: "react-aria489618031-2627"
isDisabled: undefined
isEmphasized: undefined
isQuiet: false
isReadOnly: undefined
isRequired: undefined
label: null
placeholder: "Search"
validationState: null
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
LGTM provided we are ok with pulling useProviderProps out of TextFieldBase. Left comment in that thread
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
Looks good, do we need any tests? or do we have tests that cover these?
I assumed we had tests that confirmed these props were making it to children, but if we didn't catch this in combobox. Maybe I should have a story in form and provider that looks at all the form inputs similar to the storybook stories. We do have this covered in chromatic now. |
|
That's true, chromatic would cover these now, thanks! |
|
Build successful! 🎉 |
4 participants
Closes #2323
Added useFormProps to Field so all form components have it. Audited its usage, removed in a couple places and left in a couple.
#2334 was exists to make a Chromatic baseline to fully test this.
✅ Pull Request Checklist:
📝 Test Instructions:
run chromatic and the changes should be that combobox works better in form.
Test all components that use Field because useFormProps was added which means all its children were updated.
Explicitly changed NumberField, Picker, SearchWithin, and TextFieldBase.
Check the form and provider stories.
🧢 Your Project:
RSP