Wiring up form state and value conversion on submission #2
Conversation
| /** A form field for selecting a date/time from a picker or entering it via | ||
| * keyboard. | ||
| */ | ||
| export const DatetimeInput: React.FC<InputProps> = props => { | ||
| const { label, helperText, onChange, value: propValue } = props; | ||
| const value = typeof propValue === 'string' ? propValue : defaultDate(); | ||
| const value = | ||
| typeof propValue === 'string' && propValue.length > 0 |
There was a problem hiding this comment.
Are you restricted from checking the type of this where InputProps is defined?
There was a problem hiding this comment.
InputProps.value can be one of several types, so this is guarding against what happens if we pass a type we don't expect.
I have a planned cleanup pass in the future where I may just make all input values strings to simplify the logic a bit (It's also messy in the conversion code that runs on submit)
| onChange(dateValue.toISOString()); | ||
| } else if (stringValue != null) { |
There was a problem hiding this comment.
!== or are you deliberately coercing? If the latter, would you just want else if (stringValue)?
There was a problem hiding this comment.
Deliberately coercing. I want to allow empty string through, but not null/undefined.
| }); | ||
| const options = workflowsToWorkflowSelectorOptions(workflows); | ||
| if (options.length > 0) { | ||
| options[0].description = 'latest'; |
There was a problem hiding this comment.
Is this something that is uniquely assigned here? Like, elsewhere that workflowsToWorkflowSelectorOptions is used, would you expect a nonzero length where the zeroth element is not latest? Just checking to see if this is logic that should be contained in the function itself.
There was a problem hiding this comment.
This is specific to the form. I didn't want to hard-code that logic into the selector options function.
| import { dateToTimestamp, millisecondsToDuration } from 'common/utils'; | ||
| import { Core } from 'flyteidl'; | ||
| import * as Long from 'long'; | ||
| import * as moment from 'moment'; |
There was a problem hiding this comment.
It doesn't look like this import is used. Also, I think you've capitalized Moment elsewhere when importing it in this fashion.
There was a problem hiding this comment.
It's used on line 28. My default behavior is to keep the import following the TSLint naming conventions (camelCased import). The one other place I do this, I'm importing and assigning moment.utc as moment so that all usages in the rest of the file are guaranteed to use the utc versions. I probably should do that here as well!
| /** Returns a fetchable for the default launch plan whose project/domain/name/version | ||
| * match the values in the given WorkflowId. | ||
| */ | ||
| export function useDefaultLaunchPlan(workflowId: WorkflowId | null = null) { |
There was a problem hiding this comment.
Should this be organized under hooks somewhere? Or are you keeping ones that are intended only for specific components under that component.
There was a problem hiding this comment.
I'm trying to move away from keeping all the hooks in a common folder and instead colocating hooks in the modules that use them.
|
Thanks @mrmcduff ! Since this is a feature branch, I'm going to merge and implement feedback in the next PR. |
Signed-off-by: Nastya Rusina <[email protected]>
ApiContextto do dependency injection of api functions. This lets us mock API calls cleanly in stories/tests without having to hook into Axios and do ugly protobuf encoding of mock responses.WaitForDatawhere appropriate:Literalversions for submission to the backend.