Fix issues with vendor selector behavior #4521
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Passing run #5724 ↗︎
Details:
Review all test suite changes for PR #4521 ↗︎ |
||||||||||||||||||||
| </FormControl> | ||
| ); | ||
|
|
||
| const textInput = ( |
There was a problem hiding this comment.
it's a little weird for this to be a constant as opposed to a separate React component with props passed in. not a huge deal, but I think
{isTypeahead ? <TypeaheadSelect isInvalid={isInvalid} ... /> : <TextInput isInvalid={isInvalid} ... />}is a little clearer that we are expecting components. if it feels like we are repeating too many of the same props, can also do
const props = { isInvalid: false }
{isTypeahead ? <TypeaheadSelect {...props} /> : <TextInput {...props} />}There was a problem hiding this comment.
can this component use the CustomInput shared one from inputs.tsx? or is it too customized?
There was a problem hiding this comment.
The only real difference with the CustomInput is that this one is wrapped in an InputGroup with the clear indicator as an InputRightComponent-- do you think it's reasonable to add handling for that case into the generic input?
There was a problem hiding this comment.
I think it could be! maybe if the default input is wrapped in an InputGroup (I don't think that actually renders anything but I might be wrong about that), then it could take an optional InputRightElement? it's always a balance though—we don't want to add anything too bespoke to the common components, but I think having a right element could be useful for other inputs too so if it doesn't make the code too much more complicated, I think it's worth a shot
There was a problem hiding this comment.
@allisonking After messing with putting the typeahead into its own component I'm sort of inclined to think that it's not worth it-- both components need access to things from the shared state and from useFormikContext and useField, so I'd either have to duplicate those hook calls or pass in a whole bunch (like, on the order of 10) of one-off helper methods to the typeahead as props, which I think makes it harder to read.
There was a problem hiding this comment.
okay, thanks for looking into it!
|
this is looking great @jpople ! only thing I thought was kind of weird in my testing was that the "Create new system "{word}" option goes away once you start typing more letters? I guess maybe because we've switched to the other input type? I'd expect it to keep saying "Create a new system "{word}" as I keep typing Screen.Recording.2023-12-18.at.3.42.53.PM.movI don't think it's a big deal, but that's the only thing I saw, everything else seems to meet the AC! 🎉 |
| { ...touched, vendor_id: true, name: true }, | ||
| // do not validate if a new option was created; this prevents | ||
| // incorrectly showing a "required field" error while a value is in | ||
| // the field |
There was a problem hiding this comment.
The only scenario I can think of where not validating here (and the similar call below this) would cause problems would be when manually creating a system with a taken name, but validation for name uniqueness seems to work even when shouldValidate is false here? I tried also to use Formik's manual validateField methods for this which seemed more elegant, but I couldn't make them work so this seemed fine instead.
| aria-hidden | ||
| position="absolute" | ||
| backgroundColor="transparent" | ||
| style={{ marginTop: "31.52px", marginLeft: "13px" }} |
There was a problem hiding this comment.
For some reason these margins don't get applied when using the Chakra style props, so I had to do it this way instead.
Closes #PROD-1450
Description Of Changes
Loom demo
AC of "This input is used in all places where a vendor can be selected from compass" has its own ticket PROD-1355 and is not part of this PR.
Code Changes
Steps to Confirm
With Compass enabled on the manual "Add system" form:
From a clean form:
From a clean form:
From a clean form:
Pre-Merge Checklist
CHANGELOG.md